From f7669b50eb68ba49a565d87781d071c6785cad66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20K=C3=B6gl?= Date: Mon, 14 Aug 2017 14:39:58 +0000 Subject: [PATCH 1/4] Limit number of episode actions returned by API --- doc/dev/configuration.rst | 5 +++ mygpo/api/advanced/__init__.py | 16 ++++++- mygpo/api/tests.py | 81 ++++++++++++++++++++++++++++++++++ mygpo/settings.py | 3 ++ 4 files changed, 104 insertions(+), 1 deletion(-) diff --git a/doc/dev/configuration.rst b/doc/dev/configuration.rst index dc408620d..d6a59ac5f 100644 --- a/doc/dev/configuration.rst +++ b/doc/dev/configuration.rst @@ -96,3 +96,8 @@ Social Login * ``GOOGLE_CLIENT_ID`` - Google Client ID * ``GOOGLE_CLIENT_SECRET`` - Google Client Secret + + +API +--- +* ``MAX_EPISODE_ACTIONS`` - maximum number of episode actions that the API will return in one `GET` request. diff --git a/mygpo/api/advanced/__init__.py b/mygpo/api/advanced/__init__.py index c80092524..998613e9b 100644 --- a/mygpo/api/advanced/__init__.py +++ b/mygpo/api/advanced/__init__.py @@ -146,6 +146,9 @@ def get_episode_changes(user, podcast, device, since, until, aggregated, version history = EpisodeHistoryEntry.objects.filter(user=user, timestamp__lt=until) + # return the earlier entries first + history = history.order_by('timestamp') + if since: history = history.filter(timestamp__gte=since) @@ -158,12 +161,23 @@ def get_episode_changes(user, podcast, device, since, until, aggregated, version if version == 1: history = map(convert_position, history) + max_actions = dsettings.MAX_EPISODE_ACTIONS + history = history[:max_actions+1] + + if len(history) > max_actions: + # there would have been more entries, so we return the latest + # 'timestamp' to the client + history = history[:max_actions] + timestamp = history[-1].timestamp + else: + timestamp = until + actions = [episode_action_json(a, user) for a in history] if aggregated: actions = list(dict( (a['episode'], a) for a in actions ).values()) - return {'actions': actions, 'timestamp': get_timestamp(until)} + return {'actions': actions, 'timestamp': get_timestamp(timestamp)} def episode_action_json(history, user): diff --git a/mygpo/api/tests.py b/mygpo/api/tests.py index 86504a59d..ad06a6ba3 100644 --- a/mygpo/api/tests.py +++ b/mygpo/api/tests.py @@ -1,16 +1,21 @@ import json +import uuid import copy import unittest +from datetime import datetime, timedelta from urllib.parse import urlencode from django.test.client import Client from django.test import TestCase from django.urls import reverse from django.contrib.auth import get_user_model +from django.test.utils import override_settings from mygpo.podcasts.models import Podcast, Episode from mygpo.api.advanced import episodes +from mygpo.history.models import EpisodeHistoryEntry from mygpo.test import create_auth_string, anon_request +from mygpo.utils import get_timestamp class AdvancedAPITests(unittest.TestCase): @@ -186,3 +191,79 @@ def test_episode_info(self): resp = self.client.get(url) self.assertEqual(resp.status_code, 200) + + +class EpisodeActionTests(TestCase): + + def setUp(self): + self.podcast = Podcast.objects.get_or_create_for_url( + 'http://example.com/directory-podcast.xml', + defaults = { + 'title': 'My Podcast', + }, + ) + self.episode = Episode.objects.get_or_create_for_url( + self.podcast, + 'http://example.com/directory-podcast/1.mp3', + defaults = { + 'title': 'My Episode', + }, + ) + User = get_user_model() + self.password = 'asdf' + self.username = 'adv-api-user' + self.user = User(username=self.username, email='user@example.com') + self.user.set_password(self.password) + self.user.save() + self.user.is_active = True + self.client = Client() + self.extra = { + 'HTTP_AUTHORIZATION': create_auth_string(self.username, + self.password) + } + + def tearDown(self): + self.episode.delete() + self.podcast.delete() + self.user.delete() + + @override_settings(MAX_EPISODE_ACTIONS=10) + def test_limit_actions(self): + """ Test that max MAX_EPISODE_ACTIONS episodes are returned """ + + timestamps = [] + t = datetime.utcnow() + for n in range(15): + timestamp = t - timedelta(seconds=n) + EpisodeHistoryEntry.objects.create( + timestamp = timestamp, + episode = self.episode, + user = self.user, + action = EpisodeHistoryEntry.DOWNLOAD, + ) + timestamps.append(timestamp) + + url = reverse(episodes, kwargs={ + 'version': '2', + 'username': self.user.username, + }) + response = self.client.get(url, {'since': '0'}, **self.extra) + self.assertEqual(response.status_code, 200, response.content) + response_obj = json.loads(response.content.decode('utf-8')) + actions = response_obj['actions'] + + # 10 actions should be returned + self.assertEqual(len(actions), 10) + + timestamps = sorted(timestamps) + + # the first 10 actions, according to their timestamp should be returned + for action, timestamp in zip(actions, timestamps): + self.assertEqual(timestamp.isoformat(), action['timestamp']) + + # the `timestamp` field in the response should be the timestamp of the + # last returned action + self.assertEqual( + get_timestamp(timestamps[9]), + response_obj['timestamp'] + ) diff --git a/mygpo/settings.py b/mygpo/settings.py index 2cc7e2ca4..22b6088d2 100644 --- a/mygpo/settings.py +++ b/mygpo/settings.py @@ -384,3 +384,6 @@ def get_intOrNone(name, default): '--stop', '--where=mygpo', ] + + +MAX_EPISODE_ACTIONS = int(os.getenv('MAX_EPISODE_ACTIONS', 1000)) From d751094a9a23e9aa25418593b74c1dbd9146730d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20K=C3=B6gl?= Date: Tue, 15 Aug 2017 11:52:31 +0200 Subject: [PATCH 2/4] Always return latest included timestamp in API --- mygpo/api/advanced/__init__.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/mygpo/api/advanced/__init__.py b/mygpo/api/advanced/__init__.py index 998613e9b..13fa72be4 100644 --- a/mygpo/api/advanced/__init__.py +++ b/mygpo/api/advanced/__init__.py @@ -161,22 +161,19 @@ def get_episode_changes(user, podcast, device, since, until, aggregated, version if version == 1: history = map(convert_position, history) + # Limit number of returned episode actions max_actions = dsettings.MAX_EPISODE_ACTIONS - history = history[:max_actions+1] + history = history[:max_actions] - if len(history) > max_actions: - # there would have been more entries, so we return the latest - # 'timestamp' to the client - history = history[:max_actions] - timestamp = history[-1].timestamp - else: - timestamp = until + # evaluate query and turn into list, for negative indexing + history = list(history) actions = [episode_action_json(a, user) for a in history] if aggregated: actions = list(dict( (a['episode'], a) for a in actions ).values()) + timestamp = history[-1].timestamp return {'actions': actions, 'timestamp': get_timestamp(timestamp)} From b622f2fb88e33f38614511c5cfe0801b6ae34121 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20K=C3=B6gl?= Date: Fri, 3 Aug 2018 18:45:19 +0200 Subject: [PATCH 3/4] Fix broken test --- mygpo/api/tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mygpo/api/tests.py b/mygpo/api/tests.py index 5bff249a0..9956d0864 100644 --- a/mygpo/api/tests.py +++ b/mygpo/api/tests.py @@ -201,14 +201,14 @@ def setUp(self): defaults = { 'title': 'My Podcast', }, - ) + ).object self.episode = Episode.objects.get_or_create_for_url( self.podcast, 'http://example.com/directory-podcast/1.mp3', defaults = { 'title': 'My Episode', }, - ) + ).object User = get_user_model() self.password = 'asdf' self.username = 'adv-api-user' From f334e3c21668414b1da695e48f1e044421e06cb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20K=C3=B6gl?= Date: Wed, 15 Aug 2018 20:34:27 +0200 Subject: [PATCH 4/4] Fix API response when there are no actions to return --- mygpo/api/advanced/__init__.py | 8 ++++++-- mygpo/api/tests.py | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/mygpo/api/advanced/__init__.py b/mygpo/api/advanced/__init__.py index a4b0d20df..26bb1fae9 100644 --- a/mygpo/api/advanced/__init__.py +++ b/mygpo/api/advanced/__init__.py @@ -171,8 +171,12 @@ def get_episode_changes(user, podcast, device, since, until, aggregated, version if aggregated: actions = list(dict( (a['episode'], a) for a in actions ).values()) - timestamp = history[-1].timestamp - return {'actions': actions, 'timestamp': get_timestamp(timestamp)} + if history: + ts = get_timestamp(history[-1].timestamp) + else: + ts = get_timestamp(until) + + return {'actions': actions, 'timestamp': ts} def episode_action_json(history, user): diff --git a/mygpo/api/tests.py b/mygpo/api/tests.py index 9956d0864..7f093f471 100644 --- a/mygpo/api/tests.py +++ b/mygpo/api/tests.py @@ -267,3 +267,28 @@ def test_limit_actions(self): get_timestamp(timestamps[9]), response_obj['timestamp'] ) + + + def test_no_actions(self): + """ Test when there are no actions to return """ + + t1 = get_timestamp(datetime.utcnow()) + + url = reverse(episodes, kwargs={ + 'version': '2', + 'username': self.user.username, + }) + response = self.client.get(url, {'since': '0'}, **self.extra) + self.assertEqual(response.status_code, 200, response.content) + response_obj = json.loads(response.content.decode('utf-8')) + actions = response_obj['actions'] + + # 10 actions should be returned + self.assertEqual(len(actions), 0) + + returned = response_obj['timestamp'] + t2 = get_timestamp(datetime.utcnow()) + # the `timestamp` field in the response should be the timestamp of the + # last returned action + self.assertGreaterEqual(returned, t1) + self.assertGreaterEqual(t2, returned)