From b22b9c83b08507107635f6cf7e8116a9592c881c Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 20 Jan 2020 12:13:02 +0100 Subject: [PATCH] See #170: now record downloads count on tracks/uploads --- api/config/settings/common.py | 4 ++ api/funkwhale_api/common/throttling.py | 8 ++-- api/funkwhale_api/common/views.py | 2 +- .../migrations/0048_auto_20200120_0900.py | 23 ++++++++++ api/funkwhale_api/music/models.py | 3 +- api/funkwhale_api/music/utils.py | 27 +++++++++++ api/funkwhale_api/music/views.py | 20 +++++++- api/funkwhale_api/subsonic/views.py | 1 + api/tests/common/test_throttling.py | 5 +- api/tests/music/test_utils.py | 46 +++++++++++++++++++ api/tests/music/test_views.py | 37 +++++++++++---- api/tests/subsonic/test_views.py | 3 ++ 12 files changed, 161 insertions(+), 18 deletions(-) create mode 100644 api/funkwhale_api/music/migrations/0048_auto_20200120_0900.py diff --git a/api/config/settings/common.py b/api/config/settings/common.py index 9ce7a092d..5925118b8 100644 --- a/api/config/settings/common.py +++ b/api/config/settings/common.py @@ -955,3 +955,7 @@ INSTANCE_SUPPORT_MESSAGE_DELAY = env.int("INSTANCE_SUPPORT_MESSAGE_DELAY", defau FUNKWHALE_SUPPORT_MESSAGE_DELAY = env.int("FUNKWHALE_SUPPORT_MESSAGE_DELAY", default=15) # XXX Stable release: remove USE_FULL_TEXT_SEARCH = env.bool("USE_FULL_TEXT_SEARCH", default=True) + +MIN_DELAY_BETWEEN_DOWNLOADS_COUNT = env.int( + "MIN_DELAY_BETWEEN_DOWNLOADS_COUNT", default=60 * 60 * 6 +) diff --git a/api/funkwhale_api/common/throttling.py b/api/funkwhale_api/common/throttling.py index fe4a5d934..8d37efe24 100644 --- a/api/funkwhale_api/common/throttling.py +++ b/api/funkwhale_api/common/throttling.py @@ -6,9 +6,9 @@ from rest_framework import throttling as rest_throttling from django.conf import settings -def get_ident(request): - if hasattr(request, "user") and request.user.is_authenticated: - return {"type": "authenticated", "id": request.user.pk} +def get_ident(user, request): + if user and user.is_authenticated: + return {"type": "authenticated", "id": user.pk} ident = rest_throttling.BaseThrottle().get_ident(request) return {"type": "anonymous", "id": ident} @@ -89,7 +89,7 @@ class FunkwhaleThrottle(rest_throttling.SimpleRateThrottle): def allow_request(self, request, view): self.request = request - self.ident = get_ident(request) + self.ident = get_ident(getattr(request, "user", None), request) action = getattr(view, "action", "*") view_scopes = getattr(view, "throttling_scopes", {}) if view_scopes is None: diff --git a/api/funkwhale_api/common/views.py b/api/funkwhale_api/common/views.py index b6f8d5a0a..58758773d 100644 --- a/api/funkwhale_api/common/views.py +++ b/api/funkwhale_api/common/views.py @@ -135,7 +135,7 @@ class RateLimitView(views.APIView): throttle_classes = [] def get(self, request, *args, **kwargs): - ident = throttling.get_ident(request) + ident = throttling.get_ident(getattr(request, "user", None), request) data = { "enabled": settings.THROTTLING_ENABLED, "ident": ident, diff --git a/api/funkwhale_api/music/migrations/0048_auto_20200120_0900.py b/api/funkwhale_api/music/migrations/0048_auto_20200120_0900.py new file mode 100644 index 000000000..5f9e1b466 --- /dev/null +++ b/api/funkwhale_api/music/migrations/0048_auto_20200120_0900.py @@ -0,0 +1,23 @@ +# Generated by Django 2.2.9 on 2020-01-20 09:00 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('music', '0047_auto_20200116_1246'), + ] + + operations = [ + migrations.AddField( + model_name='track', + name='downloads_count', + field=models.PositiveIntegerField(default=0), + ), + migrations.AddField( + model_name='upload', + name='downloads_count', + field=models.PositiveIntegerField(default=0), + ), + ] diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index 0d02236dd..57379453a 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -498,7 +498,7 @@ class Track(APIModelMixin): on_delete=models.SET_NULL, related_name="covered_track", ) - + downloads_count = models.PositiveIntegerField(default=0) federation_namespace = "tracks" musicbrainz_model = "recording" api = musicbrainz.api.recordings @@ -731,6 +731,7 @@ class Upload(models.Model): from_activity = models.ForeignKey( "federation.Activity", null=True, on_delete=models.SET_NULL, blank=True ) + downloads_count = models.PositiveIntegerField(default=0) objects = UploadQuerySet.as_manager() diff --git a/api/funkwhale_api/music/utils.py b/api/funkwhale_api/music/utils.py index d576ad067..5097db9ab 100644 --- a/api/funkwhale_api/music/utils.py +++ b/api/funkwhale_api/music/utils.py @@ -4,6 +4,11 @@ import magic import mutagen import pydub +from django.conf import settings +from django.core.cache import cache +from django.db.models import F + +from funkwhale_api.common import throttling from funkwhale_api.common.search import get_fts_query # noqa from funkwhale_api.common.search import get_query # noqa from funkwhale_api.common.search import normalize_query # noqa @@ -91,3 +96,25 @@ def transcode_file(input, output, input_format, output_format, **kwargs): def transcode_audio(audio, output, output_format, **kwargs): with output.open("wb"): return audio.export(output, format=output_format, **kwargs) + + +def increment_downloads_count(upload, user, wsgi_request): + ident = throttling.get_ident(user=user, request=wsgi_request) + cache_key = "downloads_count:upload-{}:{}-{}".format( + upload.pk, ident["type"], ident["id"] + ) + + value = cache.get(cache_key) + if value: + # download already tracked + return + + upload.downloads_count = F("downloads_count") + 1 + upload.track.downloads_count = F("downloads_count") + 1 + + upload.save(update_fields=["downloads_count"]) + upload.track.save(update_fields=["downloads_count"]) + + duration = max(upload.duration or 0, settings.MIN_DELAY_BETWEEN_DOWNLOADS_COUNT) + + cache.set(cache_key, 1, duration) diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py index 28babcc96..2ec0c78be 100644 --- a/api/funkwhale_api/music/views.py +++ b/api/funkwhale_api/music/views.py @@ -432,6 +432,23 @@ def get_content_disposition(filename): return "attachment; {}".format(filename) +def record_downloads(f): + def inner(*args, **kwargs): + user = kwargs.get("user") + wsgi_request = kwargs.pop("wsgi_request") + upload = kwargs.get("upload") + response = f(*args, **kwargs) + if response.status_code >= 200 and response.status_code < 400: + utils.increment_downloads_count( + upload=upload, user=user, wsgi_request=wsgi_request + ) + + return response + + return inner + + +@record_downloads def handle_serve( upload, user, format=None, max_bitrate=None, proxy_media=True, download=True ): @@ -537,12 +554,13 @@ class ListenViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet): if max_bitrate: max_bitrate = max_bitrate * 1000 return handle_serve( - upload, + upload=upload, user=request.user, format=format, max_bitrate=max_bitrate, proxy_media=settings.PROXY_MEDIA, download=download, + wsgi_request=request._request, ) diff --git a/api/funkwhale_api/subsonic/views.py b/api/funkwhale_api/subsonic/views.py index 4e70f5583..e656b5bdc 100644 --- a/api/funkwhale_api/subsonic/views.py +++ b/api/funkwhale_api/subsonic/views.py @@ -285,6 +285,7 @@ class SubsonicViewSet(viewsets.GenericViewSet): # Subsonic clients don't expect 302 redirection unfortunately, # So we have to proxy media files proxy_media=True, + wsgi_request=request._request, ) @action(detail=False, methods=["get", "post"], url_name="star", url_path="star") diff --git a/api/tests/common/test_throttling.py b/api/tests/common/test_throttling.py index 6b000fcaf..76ff2578b 100644 --- a/api/tests/common/test_throttling.py +++ b/api/tests/common/test_throttling.py @@ -10,15 +10,14 @@ def test_get_ident_anonymous(api_request): expected = {"id": ip, "type": "anonymous"} - assert throttling.get_ident(request) == expected + assert throttling.get_ident(None, request) == expected def test_get_ident_authenticated(api_request, factories): user = factories["users.User"]() request = api_request.get("/") - setattr(request, "user", user) expected = {"id": user.pk, "type": "authenticated"} - assert throttling.get_ident(request) == expected + assert throttling.get_ident(user, request) == expected @pytest.mark.parametrize( diff --git a/api/tests/music/test_utils.py b/api/tests/music/test_utils.py index 982422c34..c05b530aa 100644 --- a/api/tests/music/test_utils.py +++ b/api/tests/music/test_utils.py @@ -45,3 +45,49 @@ def test_guess_mimetype_dont_crash_with_s3(factories, mocker, settings): f = factories["music.Upload"].build(audio_file__filename="test.mp3") assert utils.guess_mimetype(f.audio_file) == "audio/mpeg" + + +def test_increment_downloads_count(factories, mocker, cache, anonymous_user, settings): + ident = {"type": "anonymous", "id": "noop"} + get_ident = mocker.patch( + "funkwhale_api.common.throttling.get_ident", return_value=ident + ) + cache_set = mocker.spy(utils.cache, "set") + wsgi_request = mocker.Mock(META={}) + upload = factories["music.Upload"]() + utils.increment_downloads_count( + upload=upload, user=anonymous_user, wsgi_request=wsgi_request + ) + + upload.refresh_from_db() + get_ident.assert_called_once_with(user=anonymous_user, request=wsgi_request) + + assert upload.downloads_count == 1 + assert upload.track.downloads_count == 1 + cache_set.assert_called_once_with( + "downloads_count:upload-{}:{}-{}".format(upload.pk, ident["type"], ident["id"]), + 1, + settings.MIN_DELAY_BETWEEN_DOWNLOADS_COUNT, + ) + + +def test_increment_downloads_count_already_tracked( + factories, mocker, cache, anonymous_user +): + ident = {"type": "anonymous", "id": "noop"} + mocker.patch("funkwhale_api.common.throttling.get_ident", return_value=ident) + wsgi_request = mocker.Mock(META={}) + upload = factories["music.Upload"]() + cache.set( + "downloads_count:upload-{}:{}-{}".format(upload.pk, ident["type"], ident["id"]), + 1, + ) + + utils.increment_downloads_count( + upload=upload, user=anonymous_user, wsgi_request=wsgi_request + ) + + upload.refresh_from_db() + + assert upload.downloads_count == 0 + assert upload.track.downloads_count == 0 diff --git a/api/tests/music/test_views.py b/api/tests/music/test_views.py index f78e6988c..68ee32b36 100644 --- a/api/tests/music/test_views.py +++ b/api/tests/music/test_views.py @@ -352,11 +352,16 @@ def test_serve_updates_access_date(factories, settings, api_client, preferences) assert upload.accessed_date > now -def test_listen_no_track(factories, logged_in_api_client): +def test_listen_no_track(factories, logged_in_api_client, mocker): + increment_downloads_count = mocker.patch( + "funkwhale_api.music.utils.increment_downloads_count" + ) + url = reverse("api:v1:listen-detail", kwargs={"uuid": "noop"}) response = logged_in_api_client.get(url) assert response.status_code == 404 + increment_downloads_count.call_count == 0 def test_listen_no_file(factories, logged_in_api_client): @@ -375,7 +380,10 @@ def test_listen_no_available_file(factories, logged_in_api_client): assert response.status_code == 404 -def test_listen_correct_access(factories, logged_in_api_client): +def test_listen_correct_access(factories, logged_in_api_client, mocker): + increment_downloads_count = mocker.patch( + "funkwhale_api.music.utils.increment_downloads_count" + ) logged_in_api_client.user.create_actor() upload = factories["music.Upload"]( library__actor=logged_in_api_client.user.actor, @@ -391,6 +399,12 @@ def test_listen_correct_access(factories, logged_in_api_client): urllib.parse.quote(expected_filename) ) + increment_downloads_count.assert_called_once_with( + upload=upload, + user=logged_in_api_client.user, + wsgi_request=response.wsgi_request, + ) + def test_listen_correct_access_download_false(factories, logged_in_api_client): logged_in_api_client.user.create_actor() @@ -419,12 +433,13 @@ def test_listen_explicit_file(factories, logged_in_api_client, mocker, settings) assert response.status_code == 200 mocked_serve.assert_called_once_with( - upload2, + upload=upload2, user=logged_in_api_client.user, format=None, max_bitrate=None, proxy_media=settings.PROXY_MEDIA, download=True, + wsgi_request=response.wsgi_request, ) @@ -484,10 +499,13 @@ def test_should_transcode_according_to_preference(value, preferences, factories) assert views.should_transcode(upload, "mp3") is expected -def test_handle_serve_create_mp3_version(factories, now): +def test_handle_serve_create_mp3_version(factories, now, mocker): + mocker.patch("funkwhale_api.music.utils.increment_downloads_count") user = factories["users.User"]() upload = factories["music.Upload"](bitrate=42) - response = views.handle_serve(upload, user, format="mp3") + response = views.handle_serve( + upload=upload, user=user, format="mp3", wsgi_request=None + ) expected_filename = upload.track.full_name + ".mp3" version = upload.versions.latest("id") @@ -514,12 +532,13 @@ def test_listen_transcode(factories, now, logged_in_api_client, mocker, settings assert response.status_code == 200 handle_serve.assert_called_once_with( - upload, + upload=upload, user=logged_in_api_client.user, format="mp3", max_bitrate=None, proxy_media=settings.PROXY_MEDIA, download=True, + wsgi_request=response.wsgi_request, ) @@ -547,12 +566,13 @@ def test_listen_transcode_bitrate( assert response.status_code == 200 handle_serve.assert_called_once_with( - upload, + upload=upload, user=logged_in_api_client.user, format=None, max_bitrate=expected, proxy_media=settings.PROXY_MEDIA, download=True, + wsgi_request=response.wsgi_request, ) @@ -578,12 +598,13 @@ def test_listen_transcode_in_place( assert response.status_code == 200 handle_serve.assert_called_once_with( - upload, + upload=upload, user=logged_in_api_client.user, format="mp3", max_bitrate=None, proxy_media=settings.PROXY_MEDIA, download=True, + wsgi_request=response.wsgi_request, ) diff --git a/api/tests/subsonic/test_views.py b/api/tests/subsonic/test_views.py index 0e7acfd66..24c66273b 100644 --- a/api/tests/subsonic/test_views.py +++ b/api/tests/subsonic/test_views.py @@ -236,6 +236,7 @@ def test_stream( format=None, max_bitrate=None, proxy_media=True, + wsgi_request=response.wsgi_request, ) assert response.status_code == 200 playable_by.assert_called_once_with(music_models.Track.objects.all(), None) @@ -256,6 +257,7 @@ def test_stream_format(format, expected, logged_in_api_client, factories, mocker format=expected, max_bitrate=None, proxy_media=True, + wsgi_request=response.wsgi_request, ) assert response.status_code == 200 @@ -305,6 +307,7 @@ def test_stream_transcode( format=expected_format, max_bitrate=expected_bitrate, proxy_media=True, + wsgi_request=response.wsgi_request, ) assert response.status_code == 200