-
-
Notifications
You must be signed in to change notification settings - Fork 219
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat(api): implement file deletion (#2960)
This implements the file delete to the Django API. Previously, the code was only manipulating the database while leaving the file in place. Co-authored-by: jo <ljonas@riseup.net>
- Loading branch information
Showing
3 changed files
with
88 additions
and
23 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,35 +1,63 @@ | ||
import os | ||
from unittest.mock import patch | ||
|
||
from django.conf import settings | ||
from model_bakery import baker | ||
from rest_framework.test import APITestCase | ||
|
||
from ...._fixtures import AUDIO_FILENAME | ||
from ...models import File | ||
|
||
|
||
class TestFileViewSet(APITestCase): | ||
@classmethod | ||
def setUpTestData(cls): | ||
cls.path = "/api/v2/files/{id}/download" | ||
cls.token = settings.CONFIG.general.api_key | ||
|
||
def test_invalid(self): | ||
path = self.path.format(id="a") | ||
def test_download_invalid(self): | ||
self.client.credentials(HTTP_AUTHORIZATION=f"Api-Key {self.token}") | ||
response = self.client.get(path) | ||
self.assertEqual(response.status_code, 400) | ||
file_id = "1" | ||
response = self.client.get(f"/api/v2/files/{file_id}/download") | ||
self.assertEqual(response.status_code, 404) | ||
|
||
def test_does_not_exist(self): | ||
path = self.path.format(id="1") | ||
def test_download(self): | ||
self.client.credentials(HTTP_AUTHORIZATION=f"Api-Key {self.token}") | ||
response = self.client.get(path) | ||
self.assertEqual(response.status_code, 404) | ||
file: File = baker.make( | ||
"storage.File", | ||
mime="audio/mp3", | ||
filepath=AUDIO_FILENAME, | ||
) | ||
response = self.client.get(f"/api/v2/files/{file.id}/download") | ||
self.assertEqual(response.status_code, 200) | ||
|
||
def test_exists(self): | ||
file = baker.make( | ||
def test_destroy(self): | ||
self.client.credentials(HTTP_AUTHORIZATION=f"Api-Key {self.token}") | ||
file: File = baker.make( | ||
"storage.File", | ||
mime="audio/mp3", | ||
filepath=AUDIO_FILENAME, | ||
) | ||
path = self.path.format(id=str(file.pk)) | ||
|
||
with patch("libretime_api.storage.views.file.remove") as remove_mock: | ||
response = self.client.delete(f"/api/v2/files/{file.id}") | ||
|
||
self.assertEqual(response.status_code, 204) | ||
remove_mock.assert_called_with( | ||
os.path.join(settings.CONFIG.storage.path, file.filepath) | ||
) | ||
|
||
def test_destroy_no_file(self): | ||
self.client.credentials(HTTP_AUTHORIZATION=f"Api-Key {self.token}") | ||
response = self.client.get(path) | ||
self.assertEqual(response.status_code, 200) | ||
file = baker.make( | ||
"storage.File", | ||
mime="audio/mp3", | ||
filepath="invalid.mp3", | ||
) | ||
response = self.client.delete(f"/api/v2/files/{file.id}") | ||
self.assertEqual(response.status_code, 204) | ||
|
||
def test_destroy_invalid(self): | ||
self.client.credentials(HTTP_AUTHORIZATION=f"Api-Key {self.token}") | ||
file_id = "1" | ||
response = self.client.delete(f"/api/v2/files/{file_id}") | ||
self.assertEqual(response.status_code, 404) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,31 +1,59 @@ | ||
import logging | ||
import os | ||
from os import remove | ||
|
||
from django.conf import settings | ||
from django.http import HttpResponse | ||
from django.shortcuts import get_object_or_404 | ||
from django.utils.encoding import filepath_to_uri | ||
from rest_framework import viewsets | ||
from rest_framework import status, viewsets | ||
from rest_framework.decorators import action | ||
from rest_framework.serializers import IntegerField | ||
from rest_framework.exceptions import APIException | ||
|
||
from ...schedule.models import Schedule | ||
from ..models import File | ||
from ..serializers import FileSerializer | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class FileInUse(APIException): | ||
status_code = status.HTTP_409_CONFLICT | ||
default_detail = "The file is currently used" | ||
default_code = "file_in_use" | ||
|
||
|
||
class FileViewSet(viewsets.ModelViewSet): | ||
queryset = File.objects.all() | ||
serializer_class = FileSerializer | ||
model_permission_name = "file" | ||
|
||
# pylint: disable=invalid-name,unused-argument | ||
@action(detail=True, methods=["GET"]) | ||
def download(self, request, pk=None): # pylint: disable=invalid-name | ||
pk = IntegerField().to_internal_value(data=pk) | ||
|
||
file = get_object_or_404(File, pk=pk) | ||
def download(self, request, pk=None): | ||
instance: File = self.get_object() | ||
|
||
response = HttpResponse() | ||
|
||
# HTTP headers must be USASCII encoded, or Nginx might not find the file and | ||
# will return a 404. | ||
redirect_uri = filepath_to_uri(os.path.join("/api/_media", file.filepath)) | ||
redirect_uri = filepath_to_uri(os.path.join("/api/_media", instance.filepath)) | ||
response["X-Accel-Redirect"] = redirect_uri | ||
return response | ||
|
||
def perform_destroy(self, instance: File): | ||
if Schedule.is_file_scheduled_in_the_future(file_id=instance.id): | ||
raise FileInUse("file is scheduled in the future") | ||
|
||
try: | ||
if instance.filepath is None: | ||
logger.warning("file does not have a filepath: %d", instance.id) | ||
return | ||
|
||
path = os.path.join(settings.CONFIG.storage.path, instance.filepath) | ||
|
||
if not os.path.isfile(path): | ||
logger.warning("file does not exist in storage: %d", instance.id) | ||
return | ||
|
||
remove(path) | ||
except OSError as exception: | ||
raise APIException("could not delete file from storage") from exception |