Skip to content
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

Fix S3Boto3Storage backend and test cases #415

Merged
merged 7 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions health_check/contrib/s3boto3_storage/backends.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
from health_check.exceptions import ServiceUnavailable

from health_check.storage.backends import StorageHealthCheck

Expand All @@ -18,7 +19,11 @@ class S3Boto3StorageHealthCheck(StorageHealthCheck):

logger = logging.getLogger(__name__)
storage = "storages.backends.s3boto3.S3Boto3Storage"
storage_alias = "default"

def check_delete(self, file_name):
storage = self.get_storage()
storage.delete(file_name)
try:
storage = self.get_storage()
storage.delete(file_name)
except Exception as e:
raise ServiceUnavailable("File was not deleted") from e

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I have a few question around this design.

What do you think of the other design:

        if not storage.exists(file_name):
            raise ServiceUnavailable("File does not exist")

...since, ServiceUnavailable is already a well-defined exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! i'll change that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have another look @50-Course if you like!

2 changes: 1 addition & 1 deletion tests/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,5 @@ def test_command_with_non_existence_subset(self):
)
stdout.seek(0)
assert stdout.read() == (
f"Specify subset: '{NON_EXISTENCE_SUBSET_NAME}' does not exists.\n"
f"Subset: '{NON_EXISTENCE_SUBSET_NAME}' does not exist.\n"
)
119 changes: 118 additions & 1 deletion tests/test_storage.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
from io import BytesIO
import unittest
from unittest import mock

import django
from django.core.files.storage import Storage
from django.test import TestCase

from health_check.contrib.s3boto3_storage.backends import S3Boto3StorageHealthCheck
from django.test import override_settings
from django.core.files.base import File
from health_check.exceptions import ServiceUnavailable
from health_check.storage.backends import (
DefaultFileStorageHealthCheck,
Expand Down Expand Up @@ -45,6 +48,62 @@ def save(self, name, content, max_length=None):
self.MOCK_FILE_COUNT += 1


# Mocking the S3Boto3Storage backend
class MockS3Boto3Storage:
"""
A mock S3Boto3Storage backend to simulate interactions with AWS S3.
"""

def __init__(self, saves=True, deletes=True):
self.saves = saves
self.deletes = deletes
self.files = {}

def open(self, name, mode="rb"):
"""
Simulates opening a file from the mocked S3 storage.
For simplicity, this doesn't differentiate between read and write modes.
"""
if name in self.files:
# Assuming file content is stored as bytes
file_content = self.files[name]
if isinstance(file_content, bytes):
return File(BytesIO(file_content))
else:
raise ValueError("File content must be bytes.")
else:
raise FileNotFoundError(f"The file {name} does not exist.")

def save(self, name, content):
"""
Overriding save to ensure content is stored as bytes in a way compatible with open method.
Assumes content is either a ContentFile, bytes, or a string that needs conversion.
"""
if self.saves:
# Check if content is a ContentFile or similar and read bytes
if hasattr(content, "read"):
file_content = content.read()
elif isinstance(content, bytes):
file_content = content
elif isinstance(content, str):
file_content = content.encode() # Convert string to bytes
else:
raise ValueError("Unsupported file content type.")

self.files[name] = file_content
return name
raise Exception("Failed to save file.")

def delete(self, name):
if self.deletes:
self.files.pop(name, None)
else:
raise Exception("Failed to delete file.")

def exists(self, name):
return name in self.files


def get_file_name(*args, **kwargs):
return "mockfile.txt"

Expand Down Expand Up @@ -156,3 +215,61 @@ def test_file_not_deleted_django_42_plus(self):
default_storage_health = DefaultFileStorageHealthCheck()
with self.assertRaises(ServiceUnavailable):
default_storage_health.check_status()


@mock.patch("storages.backends.s3boto3.S3Boto3Storage", new=MockS3Boto3Storage)
@override_settings(
STORAGES={
"default": {"BACKEND": "storages.backends.s3boto3.S3Boto3Storage"},
}
)
class HealthCheckS3Boto3StorageTests(TestCase):
"""
Tests health check behavior with a mocked S3Boto3Storage backend.
"""

def test_check_delete_success(self):
"""Test that check_delete correctly deletes a file when S3Boto3Storage is working."""
health_check = S3Boto3StorageHealthCheck()
mock_storage = health_check.get_storage()
file_name = "testfile.txt"
content = BytesIO(b"Test content")
mock_storage.save(file_name, content)

health_check.check_delete(file_name)
self.assertFalse(mock_storage.exists(file_name))

def test_check_delete_failure(self):
"""Test that check_delete raises ServiceUnavailable when deletion fails."""
with mock.patch.object(
MockS3Boto3Storage,
"delete",
side_effect=Exception("Failed to delete file."),
):
health_check = S3Boto3StorageHealthCheck()
with self.assertRaises(ServiceUnavailable):
health_check.check_delete("testfile.txt")

def test_check_status_working(self):
"""Test check_status returns True when S3Boto3Storage can save and delete files."""
health_check = S3Boto3StorageHealthCheck()
self.assertTrue(health_check.check_status())

def test_check_status_failure_on_save(self):
"""Test check_status raises ServiceUnavailable when file cannot be saved."""
with mock.patch.object(
MockS3Boto3Storage, "save", side_effect=Exception("Failed to save file.")
):
health_check = S3Boto3StorageHealthCheck()
with self.assertRaises(ServiceUnavailable):
health_check.check_status()

def test_check_status_failure_on_delete(self):
"""Test check_status raises ServiceUnavailable when file cannot be deleted."""
with mock.patch.object(
MockS3Boto3Storage, "delete", new_callable=mock.PropertyMock
) as mock_deletes:
mock_deletes.return_value = False
health_check = S3Boto3StorageHealthCheck()
with self.assertRaises(ServiceUnavailable):
health_check.check_status()