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

Respond directly with badges rather than redirecting #4559

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
35 changes: 21 additions & 14 deletions readthedocs/projects/views/public.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
from django.conf import settings
from django.contrib import messages
from django.contrib.auth.models import User
from django.contrib.staticfiles.templatetags.staticfiles import static
from django.core.cache import cache
from django.core.urlresolvers import reverse
from django.http import Http404, HttpResponse, HttpResponseRedirect
from django.shortcuts import get_object_or_404, render
from django.contrib.staticfiles.storage import staticfiles_storage
from django.views.decorators.cache import never_cache
from django.views.generic import DetailView, ListView
from taggit.models import Tag
Expand Down Expand Up @@ -117,23 +117,30 @@ def project_badge(request, project_slug):
style = "flat"
badge_path = 'projects/badges/%s-' + style + '.svg'
version_slug = request.GET.get('version', LATEST)
version = None
url = badge_path % 'unknown'

try:
version = Version.objects.public(request.user).get(
project__slug=project_slug, slug=version_slug)
except Version.DoesNotExist:
url = static(badge_path % 'unknown')
return HttpResponseRedirect(url)
version_builds = version.builds.filter(type='html',
state='finished').order_by('-date')
if not version_builds.exists():
url = static(badge_path % 'unknown')
return HttpResponseRedirect(url)
last_build = version_builds[0]
if last_build.success:
url = static(badge_path % 'passing')
else:
url = static(badge_path % 'failing')
return HttpResponseRedirect(url)
pass

if version:
version_builds = version.builds.filter(type='html',
state='finished').order_by('-date')
if version_builds.exists():
last_build = version_builds[0]
if last_build.success:
url = badge_path % 'passing'
else:
url = badge_path % 'failing'

with staticfiles_storage.open(url) as fd:
Copy link
Member

@ericholscher ericholscher Aug 23, 2018

Choose a reason for hiding this comment

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

Is there a way to put a timeout on this call? We used to do this exact same thing with shields.io, but whenever they had downtime, it took down our entire web stack because we'd end up just spinning our web processes waiting for badges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We weren't doing 302s to shields.io? I knew we used to use shields.io but I didn't look into precisely how it was implemented. Doing redirects seems like it wouldn't rely on them being up. Badges would be down when shields.io is down but it wouldn't affect our servers at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like #711 introduced shields.io. It introduced it by streaming the HTTP response from shields which is definitely brittle as you describe. When they're down, it would cause problems for us unless we specify timeouts correctly.

However, it looks like things switched to redirecting to shields.io (in this commit 47757de which doesn't really have a PR). That seems like a step in the right direction for sure. #3057 swapped shields.io for self-hosted SVGs but there's no information in the PR as to why that was desirable. To me it seems like a step back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to put a timeout on this call?

To answer this question directly, I don't see a way to add a specific timeout to this call using the version of the Azure libraries we are using. The default timeout is long too (65 seconds).

Copy link
Member

Choose a reason for hiding this comment

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

I believe shields had a decent amount of downtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this is the issue ticket about it: #2065

return HttpResponse(
fd.read(),
content_type='image/svg+xml',
Copy link
Member

Choose a reason for hiding this comment

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

Isn't here the place where we want to add some specific cache headers for lowering down the cache time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method itself has the @never_cache decorator which handles that.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect! I'm sorry, I didn't see it.

)


def project_downloads(request, project_slug):
Expand Down
6 changes: 5 additions & 1 deletion readthedocs/rtd_tests/tests/test_privacy_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
from readthedocs.oauth.models import RemoteRepository, RemoteOrganization
from readthedocs.rtd_tests.utils import create_user

from ..utils import staticfiles_test_open


class URLAccessMixin(object):

Expand Down Expand Up @@ -167,6 +169,7 @@ def setUp(self):
}


@mock.patch('django.contrib.staticfiles.storage.staticfiles_storage.open', staticfiles_test_open)
class PublicProjectMixin(ProjectMixin):

request_data = {
Expand All @@ -177,14 +180,15 @@ class PublicProjectMixin(ProjectMixin):
response_data = {
# Public
'/projects/pip/downloads/pdf/latest/': {'status_code': 302},
'/projects/pip/badge/': {'status_code': 302},
'/projects/pip/badge/': {'status_code': 200},
}

def test_public_urls(self):
from readthedocs.projects.urls.public import urlpatterns
self._test_url(urlpatterns)


@mock.patch('django.contrib.staticfiles.storage.staticfiles_storage.open', staticfiles_test_open)
class PrivateProjectMixin(ProjectMixin):

def test_private_urls(self):
Expand Down
39 changes: 21 additions & 18 deletions readthedocs/rtd_tests/tests/test_project_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@
from mock import patch
from django.test import TestCase
from django.contrib.auth.models import User
from django.contrib.staticfiles.templatetags.staticfiles import static
from django.contrib.messages import constants as message_const
from django.core.urlresolvers import reverse
from django.http.response import HttpResponseRedirect
from django.views.generic.base import ContextMixin
from django_dynamic_fixture import get
from django_dynamic_fixture import new
from django_dynamic_fixture import get, new

import six

Expand All @@ -24,6 +22,8 @@
from readthedocs.projects.views.mixins import ProjectRelationMixin
from readthedocs.projects import tasks

from ..utils import staticfiles_test_open


@patch('readthedocs.projects.views.private.trigger_build', lambda x: None)
class TestProfileMiddleware(RequestFactoryTestMixin, TestCase):
Expand Down Expand Up @@ -421,13 +421,10 @@ def get_project_queryset(self):
self.assertEqual(view.get_context_data()['project'], self.project)


@patch('django.contrib.staticfiles.storage.staticfiles_storage.open', staticfiles_test_open)
class TestBadges(TestCase):
"""Test a static badge asset is served for each build."""

# To set `flat` as default style as done in code.
def get_badge_path(self, version, style='flat'):
return static(self.BADGE_PATH % (version, style))

def setUp(self):
self.BADGE_PATH = 'projects/badges/%s-%s.svg'
self.project = get(Project, slug='badgey')
Expand All @@ -436,32 +433,38 @@ def setUp(self):

def test_unknown_badge(self):
res = self.client.get(self.badge_url, {'version': self.version.slug})
static_badge = self.get_badge_path('unknown')
self.assertEquals(res.url, static_badge)
self.assertContains(res, 'unknown')

# Unknown project
unknown_project_url = reverse('project_badge', args=['fake-project'])
res = self.client.get(unknown_project_url, {'version': 'latest'})
self.assertContains(res, 'unknown')

def test_passing_badge(self):
get(Build, project=self.project, version=self.version, success=True)
res = self.client.get(self.badge_url, {'version': self.version.slug})
static_badge = self.get_badge_path('passing')
self.assertEquals(res.url, static_badge)
self.assertContains(res, 'passing')

def test_failing_badge(self):
get(Build, project=self.project, version=self.version, success=False)
res = self.client.get(self.badge_url, {'version': self.version.slug})
static_badge = self.get_badge_path('failing')
self.assertEquals(res.url, static_badge)
self.assertContains(res, 'failing')

def test_plastic_failing_badge(self):
get(Build, project=self.project, version=self.version, success=False)
res = self.client.get(self.badge_url, {'version': self.version.slug, 'style': 'plastic'})
static_badge = self.get_badge_path('failing', 'plastic')
self.assertEquals(res.url, static_badge)
self.assertContains(res, 'failing')

# The plastic badge has slightly more rounding
self.assertContains(res, 'rx="4"')

def test_social_passing_badge(self):
get(Build, project=self.project, version=self.version, success=True)
res = self.client.get(self.badge_url, {'version': self.version.slug , 'style': 'social'})
static_badge = self.get_badge_path('passing', 'social')
self.assertEquals(res.url, static_badge)
res = self.client.get(self.badge_url, {'version': self.version.slug, 'style': 'social'})
self.assertContains(res, 'passing')

# The social badge (but not the other badges) has this element
self.assertContains(res, 'rlink')


class TestTags(TestCase):
Expand Down
14 changes: 14 additions & 0 deletions readthedocs/rtd_tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from tempfile import mkdtemp

from django.contrib.auth.models import User
from django.contrib.staticfiles import finders
from django_dynamic_fixture import new

from readthedocs.doc_builder.base import restoring_chdir
Expand Down Expand Up @@ -173,3 +174,16 @@ def create_user(username, password, **kwargs):
user.set_password(password)
user.save()
return user


def staticfiles_test_open(fp):
"""
Open a static file which may not be collected

Typically used with ``mock.patch``
"""
local_path = finders.find(fp)
if local_path:
return open(local_path)
else:
raise RuntimeError(u'Could not open file {}'.format(fp))