diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ec455b0..5d9ad14b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,17 +1,30 @@ # Changelog ## Pre-release +### Implemented enhancements +### Fixed bugs + + +## [2020.06.15](https://github.com/uktrade/directory-cms/releases/tag/2020.06.15) +[Full Changelog](https://github.com/uktrade/directory-cms/compare/2020.06.12...2020.06.15) + +### Hotfix +- No ticket - Stop using signal.alarm for bailing out of expensive code + + +## [2020.06.12](https://github.com/uktrade/directory-cms/releases/tag/2020.06.12) +[Full Changelog](https://github.com/uktrade/directory-cms/compare/2020.05.28...2020.06.12) ### Implemented enhancements - No ticket - Improve caching of markets endpoint -## Fixed bugs +### Fixed bugs - No ticket - Upgrade django to fix security vulnerability ## [2020.05.28](https://github.com/uktrade/directory-cms/releases/tag/2020.05.28) [Full Changelog](https://github.com/uktrade/directory-cms/compare/2020.05.26...2020.05.28) -## Fixed bugs +### Fixed bugs - No ticket - Upgraded dependencies to fix security vulnerability - No ticket - Expedite non-success response during cache miss diff --git a/Procfile b/Procfile index 0c7bd5d4..6a4b6cdf 100644 --- a/Procfile +++ b/Procfile @@ -1,3 +1,3 @@ -web: python manage.py distributed_migrate --noinput && gunicorn conf.wsgi --bind 0.0.0.0:$PORT +web: python manage.py distributed_migrate --noinput && gunicorn conf.wsgi --worker-class gevent --worker-connections 1000 --bind 0.0.0.0:$PORT celery_worker: FEATURE_ENFORCE_STAFF_SSO_ENABLED=False celery -A conf worker -l info celery_beat: FEATURE_ENFORCE_STAFF_SSO_ENABLED=False celery -A conf beat -l info --scheduler django_celery_beat.schedulers:DatabaseScheduler \ No newline at end of file diff --git a/conf/wsgi.py b/conf/wsgi.py index ce8d936b..847f7c67 100644 --- a/conf/wsgi.py +++ b/conf/wsgi.py @@ -11,6 +11,10 @@ from django.core.wsgi import get_wsgi_application +import psycogreen.gevent +psycogreen.gevent.patch_psycopg() + + os.environ.setdefault("DJANGO_SETTINGS_MODULE", "conf.settings") application = get_wsgi_application() diff --git a/core/cache.py b/core/cache.py index 87daeb58..c16fc80a 100644 --- a/core/cache.py +++ b/core/cache.py @@ -118,7 +118,8 @@ def populate(instance_pk): @classmethod def populate_async(cls, instance): - cls.populate.delay(instance.pk) + if instance.__class__ in MODELS_SERIALIZERS_MAPPING and instance.__class__ is not Page: + cls.populate.delay(instance.pk) @classmethod def populate_sync(cls, instance): @@ -372,6 +373,5 @@ def delete(cls, sender, instance, *args, **kwargs): @app.task def rebuild_all_cache(): for page in Page.objects.live().specific(): - if page.__class__ in MODELS_SERIALIZERS_MAPPING and page.__class__ is not Page: - CachePopulator.populate_async(page) + CachePopulator.populate_async(page) MarketPagesCachePopulator.populate() diff --git a/core/helpers.py b/core/helpers.py index 38a5cdc6..b0f6d75f 100644 --- a/core/helpers.py +++ b/core/helpers.py @@ -1,7 +1,6 @@ from num2words import num2words import copy import os -import signal from urllib.parse import urljoin import bleach @@ -246,19 +245,3 @@ def get_page_full_url(domain, full_path): def num2words_list(length): return list(map(num2words, list(range(1, length + 1)))) - - -class timeout: - def __init__(self, seconds=2, error_class=TimeoutError): - self.seconds = seconds - self.error_class = error_class - - def handle_timeout(self, signum, frame): - raise self.error_class - - def __enter__(self): - signal.signal(signal.SIGALRM, self.handle_timeout) - signal.alarm(self.seconds) - - def __exit__(self, type, value, traceback): - signal.alarm(0) diff --git a/core/views.py b/core/views.py index 63a63971..9895a854 100644 --- a/core/views.py +++ b/core/views.py @@ -27,14 +27,6 @@ logger = getLogger(__name__) -class PageNotSerializableError(NotImplementedError): - pass - - -class PageSerializationTooExpensiveError(TimeoutError): - pass - - class APIEndpointBase(PagesAdminAPIEndpoint): """At the very deep core this is a DRF GenericViewSet, with a few wagtail layers on top. @@ -50,15 +42,6 @@ class APIEndpointBase(PagesAdminAPIEndpoint): ) ) - def handle_exception(self, exc): - if isinstance(exc, PageNotSerializableError): - # page that exists has been requested, but it's not serializable. E.g, it's a folder page - return Response(status=204) - if isinstance(exc, PageSerializationTooExpensiveError): - # the request took too long so the code manually bailed out by raising a timeout - return Response(status=501) - return super().handle_exception(exc) - def get_model_class(self): if self.action == 'listing_view': model = self.get_queryset().model @@ -107,30 +90,38 @@ def detail_view(self, request, **kwargs): # Exit early if there are any issues self.check_parameter_validity() - variation_kwargs = {'lang': translation.get_language()} + draft_version = helpers.is_draft_requested(request) - if helpers.is_draft_requested(request): - variation_kwargs['draft_version'] = True + langs = [] + if translation.get_language(): + langs.append(translation.get_language()) + langs.append('en-gb') # Return a cached response if one is available - cached_data = cache.PageCache.get(page_id=self.object_id, **variation_kwargs) - if cached_data: - cached_response = helpers.CachedResponse(cached_data) - cached_response['etag'] = cached_data.get('etag', None) - return get_conditional_response( - request=request, - etag=cached_response['etag'], - response=cached_response, - ) + for lang in langs: + cached_data = cache.PageCache.get(page_id=self.object_id, lang=lang, draft_version=draft_version) + if cached_data: + cached_response = helpers.CachedResponse(cached_data) + cached_response['etag'] = cached_data.get('etag', None) + return get_conditional_response( + request=request, + etag=cached_response['etag'], + response=cached_response, + ) logger.warn('Page cache miss') - cache.CachePopulator.populate_async(self.get_object()) + page = self.get_object() + + if type(page) not in MODELS_SERIALIZERS_MAPPING: + # page that exists has been requested, but it's not serializable. E.g, it's a folder page + return Response(status=204) + else: + cache.CachePopulator.populate_async(self.get_object()) # super().detail_view can take several seconds due to unoptimized database reads - so lots of - # resources get used but ultimately will timeout. So here we attempt, but bail out after 2 seconds - with helpers.timeout(seconds=2, error_class=PageSerializationTooExpensiveError): - return super().detail_view(request, pk=None) + # resources get used but ultimately will timeout + return Response(status=501) def get_serializer_context(self): context = super().get_serializer_context() @@ -148,12 +139,6 @@ def listing_view(self, request): def object_id(self): return self.kwargs['pk'] - def get_serializer_class(self): - model_class = self.get_model_class() - if model_class not in MODELS_SERIALIZERS_MAPPING: - raise PageNotSerializableError(model_class.__name__) - return super().get_serializer_class() - class DetailViewEndpointBase(APIEndpointBase): detail_only_fields = ['id'] diff --git a/requirements.in b/requirements.in index 1a9eb3e6..47436554 100644 --- a/requirements.in +++ b/requirements.in @@ -2,7 +2,6 @@ django==2.2.* djangorestframework==3.9.* django-environ==0.4.5 gunicorn==19.5.0 -gevent==1.2.2 sentry-sdk==0.13.4 django_storages==1.7.1 whitenoise==4.1.2 @@ -36,4 +35,6 @@ notifications-python-client==5.3.* pillow>=6.* # for security fix. check compatibility on next wagtail upgrade num2words==0.5.10 pycountry==19.8.18 -elastic-apm>=5.5.2,<6.0.0 \ No newline at end of file +elastic-apm>=5.5.2,<6.0.0 +gevent>=20.6.1,<21.0.0 +psycogreen>=1.0.2,<2.0.0 diff --git a/requirements.txt b/requirements.txt index 63617f47..b0221ef9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -39,9 +39,9 @@ djangorestframework==3.9.4 # via -r requirements.in, sigauth, wagtail docopt==0.6.2 # via notifications-python-client, num2words docutils==0.16 # via botocore draftjs-exporter==2.1.7 # via wagtail -elastic-apm==5.6.0 # via -r requirements.in +elastic-apm==5.7.0 # via -r requirements.in future==0.18.2 # via celery, notifications-python-client -gevent==1.2.2 # via -r requirements.in +gevent==20.6.1 # via -r requirements.in greenlet==0.4.16 # via gevent gunicorn==19.5.0 # via -r requirements.in html2text==2018.1.9 # via -r requirements.in @@ -59,6 +59,7 @@ num2words==0.5.10 # via -r requirements.in oauthlib==3.1.0 # via requests-oauthlib packaging==20.4 # via bleach pillow==6.2.2 # via -r requirements.in, wagtail +psycogreen==1.0.2 # via -r requirements.in psycopg2==2.7.3.2 # via -r requirements.in pycountry==19.8.18 # via -r requirements.in pyjwt==1.7.1 # via notifications-python-client @@ -87,6 +88,8 @@ webencodings==0.5.1 # via bleach, html5lib whitenoise==4.1.2 # via -r requirements.in willow==1.3 # via wagtail zipp==3.1.0 # via importlib-metadata +zope.event==4.4 # via gevent +zope.interface==5.1.0 # via gevent # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/requirements_test.txt b/requirements_test.txt index 6f305799..3b06e3a5 100644 --- a/requirements_test.txt +++ b/requirements_test.txt @@ -43,13 +43,13 @@ djangorestframework==3.9.4 # via -r requirements.in, sigauth, wagtail docopt==0.6.2 # via coveralls, notifications-python-client, num2words docutils==0.16 # via botocore draftjs-exporter==2.1.7 # via wagtail -elastic-apm==5.6.0 # via -r requirements.in +elastic-apm==5.7.0 # via -r requirements.in factory-boy==2.12.0 # via -r requirements_test.in, wagtail-factories faker==4.1.0 # via factory-boy -flake8==3.8.2 # via -r requirements_test.in +flake8==3.8.3 # via -r requirements_test.in freezegun==0.3.14 # via -r requirements_test.in future==0.18.2 # via celery, notifications-python-client -gevent==1.2.2 # via -r requirements.in +gevent==20.6.1 # via -r requirements.in greenlet==0.4.16 # via gevent gunicorn==19.5.0 # via -r requirements.in html2text==2018.1.9 # via -r requirements.in @@ -69,8 +69,9 @@ num2words==0.5.10 # via -r requirements.in oauthlib==3.1.0 # via requests-oauthlib packaging==20.4 # via bleach, pytest, pytest-sugar pillow==6.2.2 # via -r requirements.in, wagtail -pip-tools==5.2.0 # via -r requirements_test.in +pip-tools==5.2.1 # via -r requirements_test.in pluggy==0.13.1 # via pytest +psycogreen==1.0.2 # via -r requirements.in psycopg2==2.7.3.2 # via -r requirements.in py==1.8.1 # via pytest pycodestyle==2.6.0 # via flake8 @@ -111,6 +112,8 @@ webencodings==0.5.1 # via bleach, html5lib whitenoise==4.1.2 # via -r requirements.in willow==1.3 # via wagtail zipp==3.1.0 # via importlib-metadata +zope.event==4.4 # via gevent +zope.interface==5.1.0 # via gevent # The following packages are considered to be unsafe in a requirements file: # pip diff --git a/tests/core/test_cache.py b/tests/core/test_cache.py index 17cea425..0e76c26f 100644 --- a/tests/core/test_cache.py +++ b/tests/core/test_cache.py @@ -242,6 +242,6 @@ def test_rebuild_all_cache_task(mock_cache_populator): article2 = InternationalArticlePageFactory(live=True) InternationalArticlePageFactory(live=False) cache.rebuild_all_cache() - assert mock_cache_populator.populate_async.call_count == 3 # contains home page + assert mock_cache_populator.populate_async.call_count == 5 # contains home page assert mock_cache_populator.populate_async.call_args_list[-2] == call(article1) assert mock_cache_populator.populate_async.call_args_list[-1] == call(article2) diff --git a/tests/core/test_views.py b/tests/core/test_views.py index 054825d7..a4dd3e10 100644 --- a/tests/core/test_views.py +++ b/tests/core/test_views.py @@ -65,6 +65,8 @@ def test_permissions_published(rf): def test_api_translations_are_loaded_when_available( client, translated_page, site_with_translated_page_as_root, language_code, expected_title ): + cache.rebuild_all_cache() + # to be added as a query params to all requests languge_query_params = {'lang': language_code} @@ -99,6 +101,7 @@ def test_api_translations_are_loaded_when_available( def test_api_falls_back_to_english_when_translations_unavailable( client, untranslated_page, site_with_untranslated_page_as_root, language_code ): + cache.rebuild_all_cache() # to be added as a query params to all requests languge_query_params = {'lang': language_code} @@ -134,9 +137,8 @@ def test_api_falls_back_to_english_when_translations_unavailable( @pytest.mark.django_db -def test_api_serves_drafts( - client, page_with_reversion, site_with_revised_page_as_root -): +def test_api_serves_drafts(client, page_with_reversion, site_with_revised_page_as_root): + cache.rebuild_all_cache() # For applying the draft token as a query param for each request param_name = permissions.DraftTokenPermisison.TOKEN_PARAM draft_query_params = { @@ -232,6 +234,7 @@ def test_upstream_anon(client, translated_page, image, url_name): def test_add_page_prepopulate( is_edit, expected_template, international_root_page, translated_page, admin_client, image, cluster_data ): + cache.rebuild_all_cache() cache.PageIDCache.populate() url = reverse('preload-add-page') model_as_dict = model_to_dict(translated_page, exclude=[ @@ -324,6 +327,8 @@ def test_page_listing(translated_page, admin_client): @pytest.mark.django_db def test_translations_exposed(translated_page, settings, client): + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': translated_page.pk}) response = client.get(url) @@ -340,6 +345,7 @@ def test_unserializable_page_requested(settings, client): depth=2, path='/thing', ) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': page.pk}) @@ -354,6 +360,8 @@ def test_lookup_by_path(international_root_page, page, admin_client): parent_page = InternationalSectorPageFactory(parent=international_root_page) page.move(target=parent_page, pos='last-child') + cache.rebuild_all_cache() + # to lookup page, the path should include the parent's slug and # the page's slug, but NOT that of app_root_page path = '/'.join([parent_page.slug, page.slug]) @@ -389,6 +397,8 @@ def test_lookup_by_path_for_non_existent_page(client): @pytest.mark.django_db def test_lookup_by_slug(translated_page, admin_client): + cache.rebuild_all_cache() + url = reverse( 'api:lookup-by-slug', kwargs={ diff --git a/tests/export_readiness/test_views.py b/tests/export_readiness/test_views.py index 32a53fb4..888edcda 100644 --- a/tests/export_readiness/test_views.py +++ b/tests/export_readiness/test_views.py @@ -5,7 +5,7 @@ from tests.export_readiness import factories from directory_constants import urls -from core.cache import MarketPagesCachePopulator +from core import cache @pytest.mark.django_db @@ -15,6 +15,7 @@ def test_performance_dashboard(admin_client, root_page): parent=root_page, product_link=urls.SERVICES_GREAT_DOMESTIC ) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': page.pk}) @@ -29,6 +30,7 @@ def test_performance_dashboard_notes(admin_client, root_page): live=True, parent=root_page ) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': page.pk}) @@ -54,6 +56,7 @@ def test_topic_landing_page_view(admin_client, root_page): parent=article_listing_page, live=True ) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': topic_landing_page.pk}) response = admin_client.get(url) @@ -75,6 +78,7 @@ def test_article_listing_page_view(admin_client, root_page): parent=article_listing_page, live=True ) + cache.rebuild_all_cache() url = reverse( 'api:api:pages:detail', kwargs={'pk': article_listing_page.pk} @@ -104,6 +108,8 @@ def test_article_page_view(admin_client, root_page): article.tags = [tag, tag2] article.save() + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': article.pk}) response = admin_client.get(url) assert response.status_code == 200 @@ -117,6 +123,7 @@ def test_domestic_homepage(admin_client, root_page): parent=root_page, hero_cta_url='/foo/bar' ) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': home_page.pk}) response = admin_client.get(url) @@ -133,6 +140,8 @@ def test_country_page_view(admin_client, root_page): tag = factories.IndustryTagFactory(name='test') page.tags = [tag] page.save() + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': page.pk}) response = admin_client.get(url) assert response.status_code == 200 @@ -166,6 +175,7 @@ def test_topic_landing_page_view_country_guides_alph_order( live=True, heading='foo' ) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': topic_landing_page.pk}) response = admin_client.get(url) @@ -232,7 +242,7 @@ def test_lookup_market_guides_missing_filters(client): market2 = factories.CountryGuidePageFactory() market2.tags = [tag] market2.save_revision().publish() - MarketPagesCachePopulator.populate() + cache.MarketPagesCachePopulator.populate() url = reverse('api:lookup-country-guides-list-view') @@ -251,7 +261,7 @@ def test_lookup_market_guides_missing_region(client): market2 = factories.CountryGuidePageFactory() market2.tags = [tag] market2.save_revision().publish() - MarketPagesCachePopulator.populate() + cache.MarketPagesCachePopulator.populate() url = reverse('api:lookup-country-guides-list-view') @@ -274,7 +284,7 @@ def test_lookup_market_guides_missing_region_markets_have_region(client): market2.save_revision().publish() factories.CountryGuidePageFactory() - MarketPagesCachePopulator.populate() + cache.MarketPagesCachePopulator.populate() url = reverse('api:lookup-country-guides-list-view') response = client.get(f'{url}?industry={tag.name},{tag2.name}') @@ -298,7 +308,7 @@ def test_lookup_market_guides_all_filters_no_cache(client): market2.save_revision() url = reverse('api:lookup-country-guides-list-view') - MarketPagesCachePopulator.populate() + cache.MarketPagesCachePopulator.populate() response = client.get(f'{url}?industry={tag.name}®ion={region.name}') assert response.status_code == 200 assert len(response.json()) == 1 @@ -316,7 +326,7 @@ def test_lookup_market_guides_all_filters_cache(client): factories.CountryGuidePageFactory(country=country) - MarketPagesCachePopulator.populate() + cache.MarketPagesCachePopulator.populate() url = reverse('api:lookup-country-guides-list-view') response = client.get(f'{url}?industry={tag.name}®ion={region.name}') diff --git a/tests/great_international/test_views.py b/tests/great_international/test_views.py index 8d81a0e4..1e32153d 100644 --- a/tests/great_international/test_views.py +++ b/tests/great_international/test_views.py @@ -4,12 +4,16 @@ from tests.great_international import factories +from core import cache + @pytest.mark.django_db def test_international_sector_page(admin_client, international_root_page): sector_page = factories.InternationalSectorPageFactory.create( parent=international_root_page ) + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': sector_page.pk}) response = admin_client.get(url) assert response.status_code == 200 @@ -20,6 +24,8 @@ def test_international_homepage(admin_client, root_page): home_page = factories.InternationalHomePageFactory.create( parent=root_page ) + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': home_page.pk}) response = admin_client.get(url) assert response.status_code == 200 @@ -34,6 +40,8 @@ def test_international_campaign_page(admin_client, international_root_page): campaign_page = factories.InternationalCampaignPageFactory( parent=international_root_page ) + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': campaign_page.pk}) response = admin_client.get(url) assert response.status_code == 200 @@ -53,6 +61,7 @@ def test_international_article_listing_page_view(admin_client, international_roo parent=article_listing_page, live=True ) + cache.rebuild_all_cache() url = reverse( 'api:api:pages:detail', kwargs={'pk': article_listing_page.pk} @@ -81,6 +90,7 @@ def test_international_topic_landing_page_view(admin_client, international_root_ parent=topic_landing_page, live=True ) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': topic_landing_page.pk}) response = admin_client.get(url) @@ -108,6 +118,8 @@ def test_international_topic_landing_page_view_sectors_alphabetical_order( live=True, heading='foo' ) + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': landing_page.pk}) response = admin_client.get(url) assert response.status_code == 200 @@ -135,6 +147,7 @@ def test_invest_home_page(document, admin_client, international_root_page): featured=False, parent=international_root_page, ) + cache.rebuild_all_cache() url = reverse( 'api:lookup-by-slug', @@ -154,6 +167,7 @@ def test_invest_region_page(admin_client, international_root_page): live=True, featured=True, parent=international_root_page ) factories.InvestRegionPageFactory(live=True, parent=page) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': page.pk}) @@ -167,6 +181,7 @@ def test_invest_region_landing_page(admin_client, international_root_page): live=True, parent=international_root_page ) factories.InvestRegionPageFactory(live=True, parent=page) + cache.rebuild_all_cache() url = reverse('api:api:pages:detail', kwargs={'pk': page.pk}) @@ -188,6 +203,7 @@ def test_high_potential_opportunity_api(document, admin_client, international_ro pdf_document=document, slug='some-nice-slug', ) + cache.rebuild_all_cache() url = reverse( 'api:lookup-by-slug', @@ -212,6 +228,8 @@ def test_international_trade_home_page_exposes_industries( homepage = factories.InternationalTradeHomePageFactory( live=True, parent=international_root_page ) + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': homepage.pk}) response = admin_client.get(url) @@ -225,6 +243,9 @@ def test_sector_page_exposes_articles_child_sub_pages(admin_client, internationa sector_page = factories.InternationalSectorPageFactory(parent=international_root_page, slug='sector-one') factories.InternationalArticlePageFactory(parent=sector_page) factories.InternationalArticlePageFactory(parent=sector_page) + + cache.rebuild_all_cache() + url = reverse('api:api:pages:detail', kwargs={'pk': sector_page.pk}) response = admin_client.get(url)