✨(backend) add deletion endpoint (#31)
* ✨(backend) add tags field for result filtering (#29) * 🚨(backend) remove dead files I left files I should have removed * ✨(backend) add tags field I add a tag field in the index and a filtering. A tag is keyword that can be applied to a document. A document can have several tags. Tags allow filtering related documents. The Conversations app needs tags for its rag tool. * ✨(backend) enhance document indexing with error handling and changelog I enhance document indexing with error handling and changelog * 📝(backend) improve docstrings and Changelog Docstrings and Changelog had to be improved. This commit improves them. * 🐛(backend) prevent information lick return user message with less info and log error * 🚨(backend) return undeleted document IDs We need better responses in case of unexpected behaviour. I enhance document deletion API by returning undeleted document IDs
This commit is contained in:
committed by
GitHub
parent
1822ee407a
commit
b0a14c4c37
@@ -27,6 +27,7 @@ and this project adheres to
|
||||
issues if the opensearch database is shared between apps.
|
||||
- ✨(backend) add tags
|
||||
- ✨(backend) adapt to conversation RAG
|
||||
- ✨(backend) add deletion endpoint
|
||||
|
||||
## Fixed
|
||||
|
||||
|
||||
@@ -117,3 +117,10 @@ class SearchQueryParametersSchema(BaseModel):
|
||||
order_by: Optional[Literal[enums.ORDER_BY_OPTIONS]] = Field(default=enums.RELEVANCE)
|
||||
order_direction: Optional[Literal["asc", "desc"]] = Field(default="desc")
|
||||
nb_results: Optional[conint(ge=1, le=300)] = Field(default=50)
|
||||
|
||||
|
||||
class DeleteDocumentsSchema(BaseModel):
|
||||
"""Schema for validating the delete documents request"""
|
||||
|
||||
service: str = Field(max_length=300)
|
||||
document_ids: List[str] = Field(min_length=1)
|
||||
|
||||
@@ -0,0 +1,210 @@
|
||||
"""Tests for deleting documents from OpenSearch over the API"""
|
||||
|
||||
import opensearchpy
|
||||
import pytest
|
||||
import responses
|
||||
from rest_framework.test import APIClient
|
||||
|
||||
from core import factories
|
||||
from core.services.opensearch import opensearch_client
|
||||
from core.utils import prepare_index
|
||||
|
||||
from .utils import build_authorization_bearer, setup_oicd_resource_server
|
||||
|
||||
pytestmark = pytest.mark.django_db
|
||||
|
||||
|
||||
def test_api_documents_delete_anonymous():
|
||||
"""Anonymous requests should not be allowed to delete documents."""
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{"service": "service-name", "document_ids": ["doc1"]},
|
||||
format="json",
|
||||
)
|
||||
|
||||
assert response.status_code == 401
|
||||
|
||||
|
||||
@responses.activate
|
||||
def test_api_documents_delete_wrong_service_name(settings):
|
||||
"""Requests with a wrong service name should return 400 Bad Request."""
|
||||
setup_oicd_resource_server(responses, settings, sub="user_sub")
|
||||
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{"service": "wrong-service", "document_ids": ["0"]},
|
||||
format="json",
|
||||
HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}",
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.json()["detail"] == "Invalid request."
|
||||
|
||||
|
||||
@responses.activate
|
||||
def test_api_documents_delete_success(settings):
|
||||
"""Authenticated users should be able to delete documents they have access to."""
|
||||
setup_oicd_resource_server(responses, settings, sub="user_sub")
|
||||
|
||||
service = factories.ServiceFactory()
|
||||
# Create documents user has access to
|
||||
documents = factories.DocumentSchemaFactory.build_batch(3, users=["user_sub"])
|
||||
prepare_index(service.index_name, documents)
|
||||
document_to_delete_ids = [doc["id"] for doc in documents[:2]]
|
||||
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{"service": service.name, "document_ids": document_to_delete_ids},
|
||||
format="json",
|
||||
HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}",
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
assert response.json()["nb-deleted-documents"] == 2
|
||||
assert response.json()["undeleted-document-ids"] == []
|
||||
|
||||
opensearch_client_ = opensearch_client()
|
||||
for document in documents:
|
||||
if document["id"] in document_to_delete_ids:
|
||||
with pytest.raises(opensearchpy.exceptions.NotFoundError):
|
||||
opensearch_client_.get(index=service.index_name, id=document["id"])
|
||||
else:
|
||||
doc = opensearch_client_.get(index=service.index_name, id=document["id"])
|
||||
assert doc["found"]
|
||||
|
||||
|
||||
@responses.activate
|
||||
def test_api_documents_delete_no_access(settings):
|
||||
"""Users should not be able to delete documents they don't have access to."""
|
||||
setup_oicd_resource_server(responses, settings, sub="user_sub")
|
||||
service = factories.ServiceFactory()
|
||||
# Create documents where user_sub does NOT have access
|
||||
documents = factories.DocumentSchemaFactory.build_batch(2, users=["other_sub"])
|
||||
prepare_index(service.index_name, documents)
|
||||
|
||||
document_ids = [doc["id"] for doc in documents]
|
||||
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{"service": service.name, "document_ids": document_ids},
|
||||
format="json",
|
||||
HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}",
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
assert response.json()["nb-deleted-documents"] == 0
|
||||
assert set(response.json()["undeleted-document-ids"]) == set(document_ids)
|
||||
|
||||
# Verify documents not deleted
|
||||
opensearch_client_ = opensearch_client()
|
||||
for doc_id in document_ids:
|
||||
doc = opensearch_client_.get(index=service.index_name, id=doc_id)
|
||||
assert doc["found"]
|
||||
|
||||
|
||||
@responses.activate
|
||||
def test_api_documents_delete_mixed_access(settings):
|
||||
"""Deleting a mix of owned and non-owned documents should only delete owned ones."""
|
||||
setup_oicd_resource_server(responses, settings, sub="user_sub")
|
||||
|
||||
service = factories.ServiceFactory()
|
||||
# Create documents with different access
|
||||
owned_documents = factories.DocumentSchemaFactory.build_batch(2, users=["user_sub"])
|
||||
other_documents = factories.DocumentSchemaFactory.build_batch(
|
||||
2, users=["other_user"]
|
||||
)
|
||||
prepare_index(service.index_name, owned_documents + other_documents)
|
||||
|
||||
owned_document_ids = [doc["id"] for doc in owned_documents]
|
||||
other_document_ids = [doc["id"] for doc in other_documents]
|
||||
non_existing_document_ids = ["non-existent-1", "non-existent-2"]
|
||||
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{
|
||||
"service": service.name,
|
||||
"document_ids": owned_document_ids
|
||||
+ other_document_ids
|
||||
+ non_existing_document_ids,
|
||||
},
|
||||
format="json",
|
||||
HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}",
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
assert response.json()["nb-deleted-documents"] == 2
|
||||
assert set(response.json()["undeleted-document-ids"]) == set(
|
||||
other_document_ids + non_existing_document_ids
|
||||
)
|
||||
|
||||
# Verify only owned documents are deleted
|
||||
opensearch_client_ = opensearch_client()
|
||||
for document_id in owned_document_ids:
|
||||
with pytest.raises(opensearchpy.exceptions.NotFoundError):
|
||||
opensearch_client_.get(index=service.index_name, id=document_id)
|
||||
|
||||
for document_id in other_document_ids:
|
||||
document = opensearch_client_.get(index=service.index_name, id=document_id)
|
||||
assert document["found"]
|
||||
|
||||
|
||||
@responses.activate
|
||||
def test_api_documents_delete_invalid_params(settings):
|
||||
"""Requests with invalid parameters should return 400 Bad Request."""
|
||||
setup_oicd_resource_server(responses, settings, sub="user_sub")
|
||||
service = factories.ServiceFactory()
|
||||
|
||||
# Missing document_ids
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{
|
||||
"service": service.name,
|
||||
},
|
||||
format="json",
|
||||
HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}",
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
|
||||
# Empty document_ids
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{"service": service.name, "document_ids": []},
|
||||
format="json",
|
||||
HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}",
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
|
||||
# Missing service
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{"document_ids": ["doc1"]},
|
||||
format="json",
|
||||
HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}",
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
|
||||
|
||||
@responses.activate
|
||||
def test_api_documents_delete_nonexistent_documents(settings):
|
||||
"""
|
||||
Deleting non-existent documents should not raise an error
|
||||
and return the list of undeleted ids.
|
||||
"""
|
||||
setup_oicd_resource_server(responses, settings, sub="user_sub")
|
||||
service = factories.ServiceFactory()
|
||||
# Create index but with no documents
|
||||
prepare_index(service.index_name, [])
|
||||
|
||||
response = APIClient().post(
|
||||
"/api/v1.0/documents/delete/",
|
||||
{"service": service.name, "document_ids": ["non-existent-id"]},
|
||||
format="json",
|
||||
HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}",
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
assert response.json()["nb-deleted-documents"] == 0
|
||||
assert response.json()["undeleted-document-ids"] == ["non-existent-id"]
|
||||
@@ -395,7 +395,7 @@ def test_api_documents_search_access__request_services(settings):
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.json() == {"detail": "Some requested services are not available"}
|
||||
assert response.json() == {"detail": "Invalid request."}
|
||||
|
||||
|
||||
@responses.activate
|
||||
@@ -419,7 +419,7 @@ def test_api_documents_search_access__request_inactive_services(settings):
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.json() == {"detail": "Service is not available"}
|
||||
assert response.json() == {"detail": "Invalid request."}
|
||||
|
||||
# Event without explicit argument, the client service from the request is not active
|
||||
response = APIClient().post(
|
||||
@@ -430,7 +430,7 @@ def test_api_documents_search_access__request_inactive_services(settings):
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.json() == {"detail": "Service is not available"}
|
||||
assert response.json() == {"detail": "Invalid request."}
|
||||
|
||||
|
||||
@responses.activate
|
||||
|
||||
@@ -2,10 +2,11 @@
|
||||
|
||||
from django.urls import include, path
|
||||
|
||||
from .views import IndexDocumentView, SearchDocumentView
|
||||
from .views import DeleteDocumentsView, IndexDocumentView, SearchDocumentView
|
||||
|
||||
urlpatterns = [
|
||||
path("documents/index/", IndexDocumentView.as_view(), name="document"),
|
||||
path("documents/search/", SearchDocumentView.as_view(), name="document"),
|
||||
path("documents/delete/", DeleteDocumentsView.as_view(), name="document"),
|
||||
path("", include("lasuite.oidc_resource_server.urls")),
|
||||
]
|
||||
|
||||
+112
-25
@@ -182,6 +182,89 @@ class IndexDocumentView(views.APIView):
|
||||
return Response(results, status=status.HTTP_201_CREATED)
|
||||
|
||||
|
||||
class DeleteDocumentsView(ResourceServerMixin, views.APIView):
|
||||
"""
|
||||
API view for deleting documents from OpenSearch.
|
||||
- Allows authenticated users to delete documents from a specified index.
|
||||
- Users can only delete documents where they are listed in the 'users' field.
|
||||
- Returns the count of deleted documents without revealing document existence.
|
||||
"""
|
||||
|
||||
authentication_classes = [ResourceServerAuthentication]
|
||||
permission_classes = [IsAuthAuthenticated]
|
||||
|
||||
def post(self, request, *args, **kwargs):
|
||||
"""
|
||||
Handle POST requests to delete documents from the specified index.
|
||||
|
||||
Only documents where the authenticated user is in the 'users' field will be deleted.
|
||||
|
||||
Body Parameters:
|
||||
---------------
|
||||
service: str
|
||||
service name to determine the index from which to delete documents.
|
||||
document_ids : List[str]
|
||||
A list of document IDs to delete from the index.
|
||||
|
||||
Returns:
|
||||
--------
|
||||
Response : rest_framework.response.Response
|
||||
- 200 OK: Returns the number of documents deleted and list of undeleted document IDs.
|
||||
- 400 Bad Request: If parameters are invalid or missing.
|
||||
"""
|
||||
params = schemas.DeleteDocumentsSchema(**request.data)
|
||||
try:
|
||||
index_name = get_opensearch_indices(
|
||||
self._get_service_provider_audience(), services=[params.service]
|
||||
)[0]
|
||||
except SuspiciousOperation as e:
|
||||
logger.error(e)
|
||||
return Response(
|
||||
{"detail": "Invalid request."},
|
||||
status=status.HTTP_400_BAD_REQUEST,
|
||||
)
|
||||
|
||||
logger.info(
|
||||
"Deleting %d documents from index %s", len(params.document_ids), index_name
|
||||
)
|
||||
|
||||
deletable_matches = opensearch_client().search(
|
||||
index=index_name,
|
||||
body={
|
||||
"query": {
|
||||
"bool": {
|
||||
"must": [
|
||||
{"ids": {"values": params.document_ids}},
|
||||
{"term": {"users": self.request.user.sub}},
|
||||
]
|
||||
}
|
||||
}
|
||||
},
|
||||
)
|
||||
deletable_ids = [hit["_id"] for hit in deletable_matches["hits"]["hits"]]
|
||||
|
||||
if deletable_ids:
|
||||
response = opensearch_client().delete_by_query(
|
||||
index=index_name,
|
||||
body={"query": {"ids": {"values": deletable_ids}}},
|
||||
)
|
||||
nb_deleted = response.get("deleted", 0)
|
||||
else:
|
||||
nb_deleted = 0
|
||||
|
||||
return Response(
|
||||
{
|
||||
"nb-deleted-documents": nb_deleted,
|
||||
"undeleted-document-ids": [
|
||||
document_id
|
||||
for document_id in params.document_ids
|
||||
if document_id not in deletable_ids
|
||||
],
|
||||
},
|
||||
status=status.HTTP_200_OK,
|
||||
)
|
||||
|
||||
|
||||
class SearchDocumentView(ResourceServerMixin, views.APIView):
|
||||
"""
|
||||
API view for searching documents in OpenSearch.
|
||||
@@ -193,27 +276,6 @@ class SearchDocumentView(ResourceServerMixin, views.APIView):
|
||||
authentication_classes = [ResourceServerAuthentication]
|
||||
permission_classes = [IsAuthAuthenticated]
|
||||
|
||||
@staticmethod
|
||||
def _get_opensearch_indices(audience, services):
|
||||
# Get request user service
|
||||
try:
|
||||
user_service = Service.objects.get(client_id=audience, is_active=True)
|
||||
except Service.DoesNotExist as e:
|
||||
logger.warning("Login failed: No service %s found", audience)
|
||||
raise SuspiciousOperation("Service is not available") from e
|
||||
|
||||
# Find allowed sub-services for this service
|
||||
allowed_services = set(user_service.services.values_list("name", flat=True))
|
||||
allowed_services.add(user_service.name)
|
||||
|
||||
if services:
|
||||
available = set(services).intersection(allowed_services)
|
||||
|
||||
if len(available) < len(services):
|
||||
raise SuspiciousOperation("Some requested services are not available")
|
||||
|
||||
return [get_opensearch_index_name(name) for name in allowed_services]
|
||||
|
||||
def post(self, request, *args, **kwargs):
|
||||
"""
|
||||
Handle POST requests to perform a search on indexed documents with optional filtering
|
||||
@@ -264,11 +326,13 @@ class SearchDocumentView(ResourceServerMixin, views.APIView):
|
||||
|
||||
# Get index list for search query
|
||||
try:
|
||||
search_indices = self._get_opensearch_indices(
|
||||
audience, services=params.services
|
||||
)
|
||||
search_indices = get_opensearch_indices(audience, services=params.services)
|
||||
except SuspiciousOperation as e:
|
||||
return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST)
|
||||
logger.error(e, exc_info=True)
|
||||
return Response(
|
||||
{"detail": "Invalid request."},
|
||||
status=status.HTTP_400_BAD_REQUEST,
|
||||
)
|
||||
|
||||
logger.info("Search '%s' on indices %s", params.q, search_indices)
|
||||
result = search(
|
||||
@@ -287,3 +351,26 @@ class SearchDocumentView(ResourceServerMixin, views.APIView):
|
||||
logger.debug("results %s", result)
|
||||
|
||||
return Response(result, status=status.HTTP_200_OK)
|
||||
|
||||
|
||||
def get_opensearch_indices(audience, services):
|
||||
"""
|
||||
Get OpenSearch indices for the given audience and services.
|
||||
"""
|
||||
try:
|
||||
user_service = Service.objects.get(client_id=audience, is_active=True)
|
||||
except Service.DoesNotExist as e:
|
||||
logger.warning("Login failed: No service %s found", audience)
|
||||
raise SuspiciousOperation("Service is not available") from e
|
||||
|
||||
# Find allowed sub-services for this service
|
||||
allowed_services = set(user_service.services.values_list("name", flat=True))
|
||||
allowed_services.add(user_service.name)
|
||||
|
||||
if services:
|
||||
available_service = set(services).intersection(allowed_services)
|
||||
|
||||
if len(available_service) < len(services):
|
||||
raise SuspiciousOperation("Some requested services are not available")
|
||||
|
||||
return [get_opensearch_index_name(service) for service in allowed_services]
|
||||
|
||||
Reference in New Issue
Block a user