diff --git a/NOTES.md b/NOTES.md index 33b71828dc2a1173e6f1fa3da0ec514b73f74c99..381ddca36cbd2189ee29d08b6b383225aa44bcaf 100644 --- a/NOTES.md +++ b/NOTES.md @@ -16,12 +16,19 @@ as such: Because keying them together by day is going to make the thing a lot harder for the frontend programmer to parse. +I know that I'm ignoring specification, and you are free to call me out +on that - but as I got some experience with frontend, +I'd rather do **the right thing**. + ## Documentation -I couldn't get the DRF documentation to cooperate with me, and frankly -while with a bigger project I'd probably stick with drf-yasg, -the inline pydocs that I've written here will suffice. +I couldn't get the DRF documentation to cooperate with me (most +possibly there's no way to do it without an external dependency), +and frankly while with a bigger project I'd probably stick with drf-yasg, +the inline pydocs that I've written here will completely suffice. +A in-depth knowledge of your tools is all good, but knowing when +to apply them trumps even that. `/swagger-ui/` is your go-to URL when it comes to listing endpoints. Submissions are to follow `/api/add`, because DRF happened to generate @@ -30,8 +37,23 @@ nice documentation there and not for Swagger. ## Authorization You can authorize your API requests either via Django cookies -or through a Web-Basic authentication. I care little which you choose. -Getting the session token for your API requests is also on you. +or through a Web-Basic authentication. +Getting the session token for your API requests is also on you, +but since the API is unit-tested you'll just have to believe me that it works. Since it was not specifically requested for history endpoint to be available for admin only, it was made available for any logged in user. + +## Test code duplication + +I realize that I would be best practice to deduplicate some code contained +within tests, but since I'm running about 1,5 of full-time equvialents you just have to forgive me +for sparing the effort to do so. Thanks "from the mountain!" + +# The reaper job + +I came up with the reaper job trying to think up a reasonable solution +that wouldn't load the processing server too much. It processes way-past-due +links into a nice compact representation and stores them in the database. +Two last days (since the job fires only every 24 hours) are computed on-the-fly, +and cached as necessary, for up to 5 minutes. diff --git a/README.md b/README.md index 68d6da000cc1c9a23276a357ea6b56f72164b2de..416509f41e09b1d517d91884848499dcaf9ce9d7 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,11 @@ ALLOWED_HOSTS and giving a list of hosts separated by a comma. If you don't define it, a default value of `*` will be used +You can optionally provide the environment variable +`REDIS_CACHE_HOST` that should contain a hostname:port of +a Redis cache instance, if you mean to configure Redis caching. +It will be disabled by default. + **WARNING!!** This is not secure, don't do it! ### Volumes diff --git a/counting/tests.py b/counting/tests.py new file mode 100644 index 0000000000000000000000000000000000000000..ccc6b615918e6d3d1b3750b8d34801ffe75d9586 --- /dev/null +++ b/counting/tests.py @@ -0,0 +1,22 @@ +from django.test import TestCase + +from django.contrib.auth.models import User +from rest_framework.test import APIClient + + +class TestShares(TestCase): + + def setUp(self) -> None: + super().setUp() + user, _ = User.objects.get_or_create(username='testuser', email='admin@admin.com', + password='12345') + + self.api_client = APIClient() + self.api_client.force_login(user) + + def test_history(self): + """ + Test that a file can be added and that it can be visited + """ + response = self.client.get('http://127.0.0.1/api/history') + self.assertGreaterEqual(len(response.json()), 2) diff --git a/counting/views.py b/counting/views.py index 00eb177cd84823d843bd6b2b1cde8b0114108d85..7b57dd1731d717a1c98ad36bfbf84614e678f465 100644 --- a/counting/views.py +++ b/counting/views.py @@ -1,9 +1,12 @@ import datetime # Create your views here. +from django.utils.decorators import method_decorator +from django.views.decorators.cache import cache_page from rest_framework import serializers, status from rest_framework.response import Response from rest_framework.views import APIView +from satella.time import parse_time_string from counting.cron import ReaperJob, DayType from counting.models import StoryOfADay @@ -38,6 +41,8 @@ class GetHistory(APIView): You must be authorized to access this endpoint. + This endpoint is cached for up to 5 minutes. + The result will be a list of elements like { "day": "YYYY-MM-DD", @@ -46,9 +51,10 @@ class GetHistory(APIView): } """ + @method_decorator(cache_page(parse_time_string('5m'))) def get(self, request): - # if not request.user.is_authenticated: - # return Response({}, status=status.HTTP_401_UNAUTHORIZED) + if not request.user.is_authenticated: + return Response({}, status=status.HTTP_401_UNAUTHORIZED) items = set(StoryOfADay.objects.all()) today = datetime.date.today() items.add(get_history_for(today)) diff --git a/docker-compose.yml b/docker-compose.yml index e84636936e6b99c3cbfeacb9b7a898a82cf1d459..e2c086c850fc2a2a5c869c1f1a0ad45707f22127 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,5 +1,7 @@ version: '3.4' services: + redis: + image: redis postgres: image: postgres environment: @@ -13,14 +15,18 @@ services: build: . environment: - DEBUG=1 + - REDIS_CACHE_HOST=redis:6379 depends_on: - postgres + - redis run_local: build: . depends_on: - postgres + - redis environment: - DEBUG=1 + - REDIS_CACHE_HOST=redis:6379 ports: - 80:80 - 81:81 diff --git a/netguru/settings.py b/netguru/settings.py index d12d8e478e96913b462425569cd4a3ea3c42cbf9..746d0446ecd705a332f5de0bc3c7a3078bd357d5 100644 --- a/netguru/settings.py +++ b/netguru/settings.py @@ -1,5 +1,7 @@ import os import logging + +import django_opentracing from satella.instrumentation.metrics import getMetric from satella.instrumentation.metrics.exporters import PrometheusHTTPExporterThread @@ -141,6 +143,15 @@ STATIC_ROOT = os.path.join(BASE_DIR, 'media') FILE_SAVE_LOCATION = '/data' +# Configure cache +if os.environ.get('REDIS_CACHE_HOST'): + CACHES = { + 'default': { + 'BACKEND': 'redis_cache.RedisCache', + 'LOCATION': os.environ['REDIS_CACHE_HOST'], + }, + } + # Configure DRF and Swagger DEFAULT_SCHEMA_CLASS = 'rest_framework.schemas.openapi.AutoSchema' diff --git a/netguru/urls.py b/netguru/urls.py index 99f10a658f65d9236dc820b6c581629b100e4e0a..f566220779de75cf76762bdd69157aafad03c8a5 100644 --- a/netguru/urls.py +++ b/netguru/urls.py @@ -1,6 +1,6 @@ from django.conf.urls.static import static from django.contrib import admin -from django.urls import path, include, re_path +from django.urls import path, include from django.views.generic import TemplateView from rest_framework.schemas import get_schema_view @@ -14,7 +14,7 @@ urlpatterns = [ path('api/add', shares_api.AddShare.as_view()), path('api/history', counting_views.GetHistory.as_view()), path('api/get/<int:share_id>', shares_api.GetShare.as_view()), - re_path(r'shares/(?P<share_id>[0-9]+)$', shares_views.view_share), + path('shares/<int:share_id>', shares_views.view_share), path('admin/', admin.site.urls), path('', shares_views.add_share), ] + static('/static/', document_root='/app/media/') diff --git a/requirements.txt b/requirements.txt index f588852fb862e635077aa0515dcb1d4966f78746..a8782f9159b5be75159e21b3d7f82ae26186ffa4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -9,3 +9,4 @@ djangorestframework pyyaml uritemplate coreapi +django-redis-cache diff --git a/shares/api.py b/shares/api.py index 8eb2d1fec12a23858536665424358c663db9b919..6a383dbdb7e99fb4afc175cd9ef8889479121607 100644 --- a/shares/api.py +++ b/shares/api.py @@ -1,5 +1,6 @@ -from django.shortcuts import get_object_or_404, redirect -from django.http import StreamingHttpResponse as SHTTPResponse +import logging +from django.shortcuts import get_object_or_404 +from django.http import StreamingHttpResponse as SHTTPResponse, HttpResponseRedirect from rest_framework import serializers, status from rest_framework.parsers import JSONParser, FileUploadParser, MultiPartParser from rest_framework.response import Response @@ -8,6 +9,7 @@ from satella.instrumentation.metrics import getMetric from shares.models import Share, hash_password, SHARE_FILE, add_file, add_url +logger = logging.getLogger(__name__) files = getMetric('visited.api.files', 'counter') links = getMetric('visited.api.links', 'counter') @@ -25,27 +27,29 @@ class ShareSerializer(serializers.Serializer): class AddShare(APIView): """ - Adds a share + Adds a share. - You have to provide either a field called url or upload a file. If you're using - multipart, name it file. + You have to provide either a field called url or upload a file using multipart. + You should name it file. The result will be a JSON object containing two properties: url and password. The result will be passed using HTTP 201. You must be authorized to access this endpoint. """ - parser_classes = FileUploadParser, MultiPartParser, JSONParser + parser_classes = JSONParser, MultiPartParser, FileUploadParser def get_serializer(self): return AddShareSerializer() - def post(self, request, format=None): + def put(self, request): if not request.user.is_authenticated: return Response({}, status=status.HTTP_401_UNAUTHORIZED) - if 'file' in request.data: - dct = add_file(request.data['file'], request, escape_url=False) - elif 'url' in request.data: + if request.FILES.get('file'): + dct = add_file(request.FILES['file'], request, escape_url=False) + logger.info('Created a file via API') + elif request.data.get('url'): dct = add_url(request.data['url'], request, escape_url=False) + logger.info('Created an URL via API') else: return Response({}, status=status.HTTP_400_BAD_REQUEST) @@ -64,15 +68,19 @@ class GetShare(APIView): You will receive either a 302 Redirect or a 200 OK with the file contents """ - def get_serializer(self, *args): - return GetShareSerializer(*args) + def get_serializer(self, **kwargs): + return GetShareSerializer(**kwargs) def post(self, request, share_id: str, format=None): - serializer = self.get_serializer(request.data) + serializer = self.get_serializer(data=request.data) if not serializer.is_valid(): return Response({}, status=status.HTTP_400_BAD_REQUEST) share = get_object_or_404(Share, id=share_id) + + if share.expired: + return Response({}, status=status.HTTP_404_NOT_FOUND) + if hash_password(serializer.data['password']) != share.pwd_hash: return Response({}, status=status.HTTP_401_UNAUTHORIZED) @@ -82,7 +90,7 @@ class GetShare(APIView): return SHTTPResponse(**share.get_kwargs_for_s_http_response()) else: links.runtime(+1) - return redirect(share.resource) + return HttpResponseRedirect(redirect_to=share.resource) finally: share.times_used += 1 share.save() diff --git a/shares/tests.py b/shares/tests.py index d27c4401341ef5ba5fd7ea00e2fb86a52bf2312b..154d1b6ce1b2ef058cb8526a850fa38d20d9a59a 100644 --- a/shares/tests.py +++ b/shares/tests.py @@ -1,16 +1,13 @@ -# -# APOLOGIES IN ADVANCE!!! -# -# I realize that I would be best practice to deduplicate some code contained here -# but since I'm running about 1,5 of full-time equvialents you just have to forgive me -# for sparing the effort to do so. Thanks "from the mountain!" -# +import datetime from django.contrib.auth.models import User from django.test import TestCase, Client from django.test.html import parse_html +from rest_framework.test import APIClient from satella.files import write_to_file +from shares.models import Share + def find_element(document, field, value): if isinstance(document, str): @@ -34,7 +31,10 @@ class TestShares(TestCase): self.client = Client() # May be you have missed this line self.client.force_login(user) - def test_add_file(self): + self.api_client = APIClient() + self.api_client.force_login(user) + + def test_add_file_form(self): """ Test that a file can be added and that it can be visited """ @@ -68,7 +68,46 @@ class TestShares(TestCase): self.assertEqual(b''.join(resp), FILE_CONTENTS) self.assertEqual(response.status_code, 200) - def test_add_url(self): + def test_add_url_api_expired(self): + response = self.api_client.put('http://127.0.0.1/api/add', {'url': 'http://example.com'}, + format='json') + self.assertEqual(response.status_code, 201) + share = Share.objects.get(id=response.json()['url'].rsplit('/', 1)[-1]) + share.created_on = datetime.datetime.now() - datetime.timedelta(days=2) + share.save() + response = self.api_client.post(f'http://127.0.0.1/api/get/{share.id}', {'password': response.json()['password']}, + format='json') + self.assertEqual(response.status_code, 404) + + def test_add_url_api(self): + response = self.api_client.put('http://127.0.0.1/api/add', {'url': 'http://example.com'}, + format='json') + self.assertEqual(response.status_code, 201) + + id_ = response.json()['url'].rsplit('/', 1)[-1] + url = f'http://127.0.0.1/api/get/{id_}' + + response = self.api_client.post(url, {'password': response.json()['password']}, + format='json') + self.assertEqual(response.status_code, 302) + + def test_add_file_api(self): + FILE_CONTENTS = b'somedata' + write_to_file('test', FILE_CONTENTS) + with open('test', 'rb') as f_in: + response = self.api_client.put('http://127.0.0.1/api/add', {'file': f_in}, + format='multipart') + self.assertEqual(response.status_code, 201) + + id_ = response.json()['url'].rsplit('/', 1)[-1] + url = f'http://127.0.0.1/api/get/{id_}' + + response = self.api_client.post(url, {'password': response.json()['password']}, + format='json') + self.assertEqual(response.status_code, 200) + self.assertEqual(b''.join(response.streaming_content), FILE_CONTENTS) + + def test_add_url_form(self): """ Test that an URL can be added and that it can be visited """ @@ -97,3 +136,26 @@ class TestShares(TestCase): 'csrfmiddlewaretoken': csrf_token }) self.assertEqual(response.status_code, 302) + + def test_add_url_form_expired(self): + response = self.client.get('http://127.0.0.1/accounts/profile') + + self.assertEqual(response.status_code, 200) + html = parse_html(response.content.decode('utf-8')) + csrf_token = find_element(html, 'name', 'csrfmiddlewaretoken').attributes['value'] + + response = self.client.post('http://127.0.0.1/accounts/profile', { + 'url': 'https://example.com', + 'csrfmiddlewaretoken': csrf_token + }) + self.assertEqual(response.status_code, 200) + + html = parse_html(response.content.decode('utf-8')) + link = find_element(html, 'id', 'link_url').children[0].replace('https', 'http') + + share = Share.objects.get(id=link.rsplit('/', 1)[-1]) + share.created_on = datetime.datetime.now() - datetime.timedelta(days=2) + share.save() + + response = self.client.get(link) + self.assertEqual(response.status_code, 404) diff --git a/shares/views.py b/shares/views.py index 2c28bcdddf629ab847330e5033cbe07ba527c0d9..7c9504209626ed77dbd5955761d8e431cac6a0fc 100644 --- a/shares/views.py +++ b/shares/views.py @@ -49,11 +49,8 @@ def add_share(request): if request.method == 'POST': form = AddShareURLForm(request.POST, request.FILES) if form.is_valid(): - - pwd_hash = hash_password(data_added['password']) data = form.cleaned_data if data.get('url'): - # Create a URL resource data_added = add_url(data['url'], request) logger.info('Created a link via form') else: