diff --git a/readthedocs/builds/migrations/0006_add_config_field.py b/readthedocs/builds/migrations/0006_add_config_field.py new file mode 100644 index 00000000000..7af36e8ad79 --- /dev/null +++ b/readthedocs/builds/migrations/0006_add_config_field.py @@ -0,0 +1,21 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.16 on 2018-11-02 13:24 +from __future__ import unicode_literals + +from django.db import migrations +import jsonfield.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('builds', '0005_remove-version-alias'), + ] + + operations = [ + migrations.AddField( + model_name='build', + name='_config', + field=jsonfield.fields.JSONField(default=dict, verbose_name='Configuration used in the build'), + ), + ] diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index ec2825e44e4..ab93ec9d81a 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -17,10 +17,12 @@ from django.conf import settings from django.core.urlresolvers import reverse from django.db import models +from django.utils import timezone from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext from django.utils.translation import ugettext_lazy as _ from guardian.shortcuts import assign +from jsonfield import JSONField from taggit.managers import TaggableManager from readthedocs.core.utils import broadcast @@ -128,6 +130,21 @@ def __str__(self): pk=self.pk, )) + @property + def config(self): + """ + Proxy to the configuration of the build. + + :returns: The configuration used in the last successful build. + :rtype: dict + """ + last_build = ( + self.builds.filter(state='finished', success=True) + .order_by('-date') + .first() + ) + return last_build.config + @property def commit_name(self): """ @@ -450,6 +467,7 @@ class Build(models.Model): exit_code = models.IntegerField(_('Exit code'), null=True, blank=True) commit = models.CharField( _('Commit'), max_length=255, null=True, blank=True) + _config = JSONField(_('Configuration used in the build'), default=dict) length = models.IntegerField(_('Build Length'), null=True, blank=True) @@ -463,11 +481,81 @@ class Build(models.Model): objects = BuildQuerySet.as_manager() + CONFIG_KEY = '__config' + class Meta(object): ordering = ['-date'] get_latest_by = 'date' index_together = [['version', 'state', 'type']] + def __init__(self, *args, **kwargs): + super(Build, self).__init__(*args, **kwargs) + self._config_changed = False + + @property + def previous(self): + """ + Returns the previous build to the current one. + + Matching the project and version. + """ + date = self.date or timezone.now() + if self.project is not None and self.version is not None: + return ( + Build.objects + .filter( + project=self.project, + version=self.version, + date__lt=date, + ) + .order_by('-date') + .first() + ) + return None + + @property + def config(self): + """ + Get the config used for this build. + + Since we are saving the config into the JSON field only when it differs + from the previous one, this helper returns the correct JSON used in + this Build object (it could be stored in this object or one of the + previous ones). + """ + if self.CONFIG_KEY in self._config: + return Build.objects.get(pk=self._config[self.CONFIG_KEY])._config + return self._config + + @config.setter + def config(self, value): + """ + Set `_config` to value. + + `_config` should never be set directly from outside the class. + """ + self._config = value + self._config_changed = True + + def save(self, *args, **kwargs): # noqa + """ + Save object. + + To save space on the db we only save the config if it's different + from the previous one. + + If the config is the same, we save the pk of the object + that has the **real** config under the `CONFIG_KEY` key. + """ + if self.pk is None or self._config_changed: + previous = self.previous + if (previous is not None and + self._config and self._config == previous.config): + previous_pk = previous._config.get(self.CONFIG_KEY, previous.pk) + self._config = {self.CONFIG_KEY: previous_pk} + super(Build, self).save(*args, **kwargs) + self._config_changed = False + def __str__(self): return ugettext( 'Build {project} for {usernames} ({pk})'.format( diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 9fe11087bf1..bcecddecae6 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -142,6 +142,11 @@ class BuildConfigBase(object): from another source (like the web admin). """ + PUBLIC_ATTRIBUTES = [ + 'version', 'formats', 'python', + 'conda', 'build', 'doctype', + 'sphinx', 'mkdocs', 'submodules', + ] version = None def __init__(self, env_config, raw_config, source_file, source_position): @@ -248,6 +253,16 @@ def python_full_version(self): ) return ver + def as_dict(self): + config = {} + for name in self.PUBLIC_ATTRIBUTES: + attr = getattr(self, name) + if hasattr(attr, '_asdict'): + config[name] = attr._asdict() + else: + config[name] = attr + return config + def __getattr__(self, name): """Raise an error for unknown attributes.""" raise ConfigOptionNotSupportedError(name) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 3581109324b..a95c36cbc4e 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -840,6 +840,60 @@ def test_config_filenames_regex(correct_config_filename): assert re.match(CONFIG_FILENAME_REGEX, correct_config_filename) +def test_as_dict(tmpdir): + apply_fs(tmpdir, {'requirements.txt': ''}) + build = get_build_config( + { + 'version': 1, + 'formats': ['pdf'], + 'python': { + 'version': 3.5, + }, + 'requirements_file': 'requirements.txt', + }, + get_env_config({ + 'defaults': { + 'doctype': 'sphinx', + 'sphinx_configuration': None, + }, + }), + source_file=str(tmpdir.join('readthedocs.yml')), + ) + build.validate() + expected_dict = { + 'version': '1', + 'formats': ['pdf'], + 'python': { + 'version': 3.5, + 'requirements': 'requirements.txt', + 'install_with_pip': False, + 'install_with_setup': False, + 'extra_requirements': [], + 'use_system_site_packages': False, + }, + 'build': { + 'image': 'readthedocs/build:2.0', + }, + 'conda': None, + 'sphinx': { + 'builder': 'sphinx', + 'configuration': None, + 'fail_on_warning': False, + }, + 'mkdocs': { + 'configuration': None, + 'fail_on_warning': False, + }, + 'doctype': 'sphinx', + 'submodules': { + 'include': ALL, + 'exclude': [], + 'recursive': True, + }, + } + assert build.as_dict() == expected_dict + + class TestBuildConfigV2(object): def get_build_config(self, config, env_config=None, @@ -1811,3 +1865,47 @@ def test_pop_config_raise_exception(self): build.pop_config('one.four', raise_ex=True) assert excinfo.value.value == 'four' assert excinfo.value.code == VALUE_NOT_FOUND + + def test_as_dict(self, tmpdir): + apply_fs(tmpdir, {'requirements.txt': ''}) + build = self.get_build_config( + { + 'version': 2, + 'formats': ['pdf'], + 'python': { + 'version': 3.6, + 'requirements': 'requirements.txt', + }, + }, + source_file=str(tmpdir.join('readthedocs.yml')), + ) + build.validate() + expected_dict = { + 'version': '2', + 'formats': ['pdf'], + 'python': { + 'version': 3.6, + 'requirements': str(tmpdir.join('requirements.txt')), + 'install_with_pip': False, + 'install_with_setup': False, + 'extra_requirements': [], + 'use_system_site_packages': False, + }, + 'build': { + 'image': 'readthedocs/build:latest', + }, + 'conda': None, + 'sphinx': { + 'builder': 'sphinx', + 'configuration': None, + 'fail_on_warning': False, + }, + 'mkdocs': None, + 'doctype': 'sphinx', + 'submodules': { + 'include': [], + 'exclude': ALL, + 'recursive': False, + }, + } + assert build.as_dict() == expected_dict diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 5b8dbf7a40c..7ccc5b8ebe7 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -406,8 +406,10 @@ def run_setup(self, record=True): raise YAMLParseError( YAMLParseError.GENERIC_WITH_PARSE_EXCEPTION.format( exception=str(e), - )) + ) + ) + self.save_build_config() self.additional_vcs_operations() if self.setup_env.failure or self.config is None: @@ -531,9 +533,14 @@ def get_build(build_pk): build = {} if build_pk: build = api_v2.build(build_pk).get() - return dict((key, val) for (key, val) in list(build.items()) - if key not in ['project', 'version', 'resource_uri', - 'absolute_uri']) + private_keys = [ + 'project', 'version', 'resource_uri', 'absolute_uri' + ] + return { + key: val + for key, val in build.items() + if key not in private_keys + } def setup_vcs(self): """ @@ -594,6 +601,15 @@ def set_valid_clone(self): self.project.has_valid_clone = True self.version.project.has_valid_clone = True + def save_build_config(self): + """Save config in the build object.""" + pk = self.build['id'] + config = self.config.as_dict() + api_v2.build(pk).patch({ + 'config': config, + }) + self.build['config'] = config + def update_app_instances(self, html=False, localmedia=False, search=False, pdf=False, epub=False): """ diff --git a/readthedocs/restapi/serializers.py b/readthedocs/restapi/serializers.py index 8517899f54c..0cf346c6d8b 100644 --- a/readthedocs/restapi/serializers.py +++ b/readthedocs/restapi/serializers.py @@ -112,10 +112,14 @@ class BuildSerializer(serializers.ModelSerializer): version_slug = serializers.ReadOnlyField(source='version.slug') docs_url = serializers.ReadOnlyField(source='version.get_absolute_url') state_display = serializers.ReadOnlyField(source='get_state_display') + # Jsonfield needs an explicit serializer + # https://github.com/dmkoch/django-jsonfield/issues/188#issuecomment-300439829 + config = serializers.JSONField(required=False) class Meta(object): model = Build - exclude = ('builder',) + # `_config` should be excluded to avoid conflicts with `config` + exclude = ('builder', '_config') class BuildAdminSerializer(BuildSerializer): @@ -123,7 +127,8 @@ class BuildAdminSerializer(BuildSerializer): """Build serializer for display to admin users and build instances.""" class Meta(BuildSerializer.Meta): - exclude = () + # `_config` should be excluded to avoid conflicts with `config` + exclude = ('_config',) class SearchIndexSerializer(serializers.Serializer): diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 0f277d841fd..2981bf5b137 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -56,6 +56,7 @@ def test_make_build(self): self.assertEqual(resp.status_code, status.HTTP_201_CREATED) build = resp.data self.assertEqual(build['state_display'], 'Cloning') + self.assertEqual(build['config'], {}) resp = client.get('/api/v2/build/%s/' % build['id']) self.assertEqual(resp.status_code, 200) @@ -63,6 +64,132 @@ def test_make_build(self): self.assertEqual(build['output'], 'Test Output') self.assertEqual(build['state_display'], 'Cloning') + def test_api_does_not_have_private_config_key_superuser(self): + client = APIClient() + client.login(username='super', password='test') + project = Project.objects.get(pk=1) + version = project.versions.first() + build = Build.objects.create(project=project, version=version) + + resp = client.get('/api/v2/build/{}/'.format(build.pk)) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertIn('config', resp.data) + self.assertNotIn('_config', resp.data) + + def test_api_does_not_have_private_config_key_normal_user(self): + client = APIClient() + project = Project.objects.get(pk=1) + version = project.versions.first() + build = Build.objects.create(project=project, version=version) + + resp = client.get('/api/v2/build/{}/'.format(build.pk)) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertIn('config', resp.data) + self.assertNotIn('_config', resp.data) + + def test_save_config(self): + client = APIClient() + client.login(username='super', password='test') + resp = client.post( + '/api/v2/build/', + { + 'project': 1, + 'version': 1, + 'config': {'one': 'two'}, + }, + format='json', + ) + self.assertEqual(resp.status_code, status.HTTP_201_CREATED) + build_one = resp.data + self.assertEqual(build_one['config'], {'one': 'two'}) + + resp = client.get('/api/v2/build/%s/' % build_one['id']) + self.assertEqual(resp.status_code, 200) + build = resp.data + self.assertEqual(build['config'], {'one': 'two'}) + + def test_save_same_config(self): + client = APIClient() + client.login(username='super', password='test') + resp = client.post( + '/api/v2/build/', + { + 'project': 1, + 'version': 1, + 'config': {'one': 'two'}, + }, + format='json', + ) + self.assertEqual(resp.status_code, status.HTTP_201_CREATED) + build_one = resp.data + self.assertEqual(build_one['config'], {'one': 'two'}) + + resp = client.post( + '/api/v2/build/', + { + 'project': 1, + 'version': 1, + 'config': {'one': 'two'}, + }, + format='json', + ) + self.assertEqual(resp.status_code, status.HTTP_201_CREATED) + build_two = resp.data + self.assertEqual(build_two['config'], {'one': 'two'}) + + resp = client.get('/api/v2/build/%s/' % build_one['id']) + self.assertEqual(resp.status_code, 200) + build = resp.data + self.assertEqual(build['config'], {'one': 'two'}) + + # Checking the values from the db, just to be sure the + # api isn't lying. + self.assertEqual( + Build.objects.get(pk=build_one['id'])._config, + {'one': 'two'}, + ) + self.assertEqual( + Build.objects.get(pk=build_two['id'])._config, + {Build.CONFIG_KEY: build_one['id']}, + ) + + def test_save_same_config_using_patch(self): + client = APIClient() + client.login(username='super', password='test') + project = Project.objects.get(pk=1) + version = project.versions.first() + build_one = Build.objects.create(project=project, version=version) + resp = client.patch( + '/api/v2/build/{}/'.format(build_one.pk), + {'config': {'one': 'two'}}, + format='json', + ) + self.assertEqual(resp.data['config'], {'one': 'two'}) + + build_two = Build.objects.create(project=project, version=version) + resp = client.patch( + '/api/v2/build/{}/'.format(build_two.pk), + {'config': {'one': 'two'}}, + format='json', + ) + self.assertEqual(resp.data['config'], {'one': 'two'}) + + resp = client.get('/api/v2/build/{}/'.format(build_one.pk)) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + build = resp.data + self.assertEqual(build['config'], {'one': 'two'}) + + # Checking the values from the db, just to be sure the + # api isn't lying. + self.assertEqual( + Build.objects.get(pk=build_one.pk)._config, + {'one': 'two'}, + ) + self.assertEqual( + Build.objects.get(pk=build_two.pk)._config, + {Build.CONFIG_KEY: build_one.pk}, + ) + def test_response_building(self): """ The ``view docs`` attr should return a link diff --git a/readthedocs/rtd_tests/tests/test_builds.py b/readthedocs/rtd_tests/tests/test_builds.py index 31b4d348fd7..6e91ffba12e 100644 --- a/readthedocs/rtd_tests/tests/test_builds.py +++ b/readthedocs/rtd_tests/tests/test_builds.py @@ -1,17 +1,19 @@ -from __future__ import absolute_import +from __future__ import ( + absolute_import, + division, + print_function, + unicode_literals, +) import mock -import six - from django.test import TestCase -from django_dynamic_fixture import get -from django_dynamic_fixture import fixture +from django_dynamic_fixture import fixture, get -from readthedocs.projects.models import Project +from readthedocs.builds.models import Build, Version from readthedocs.doc_builder.config import load_yaml_config from readthedocs.doc_builder.environments import LocalBuildEnvironment from readthedocs.doc_builder.python_environments import Virtualenv -from readthedocs.doc_builder.loader import get_builder_class +from readthedocs.projects.models import Project from readthedocs.projects.tasks import UpdateDocsTaskStep from readthedocs.rtd_tests.tests.test_config_integration import create_load @@ -223,3 +225,198 @@ def test_build_pdf_latex_not_failure(self, load_config): task.build_docs() self.assertEqual(self.mocks.popen.call_count, 7) self.assertTrue(build_env.successful) + + @mock.patch('readthedocs.projects.tasks.api_v2') + @mock.patch('readthedocs.doc_builder.config.load_config') + def test_save_config_in_build_model(self, load_config, api_v2): + load_config.side_effect = create_load() + api_v2.build.get.return_value = {} + project = get( + Project, + slug='project', + documentation_type='sphinx', + ) + build = get(Build) + version = get(Version, slug='1.8', project=project) + task = UpdateDocsTaskStep( + project=project, version=version, build={'id': build.pk} + ) + task.setup_vcs = mock.Mock() + task.run_setup() + build_config = task.build['config'] + # For patch + api_v2.build.assert_called_once() + assert build_config['version'] == '1' + assert 'sphinx' in build_config + assert build_config['doctype'] == 'sphinx' + + +class BuildModelTests(TestCase): + + def setUp(self): + self.project = get(Project) + self.version = get(Version, project=self.project) + + def test_get_previous_build(self): + build_one = get( + Build, + project=self.project, + version=self.version, + config={'version': 1} + ) + build_two = get( + Build, + project=self.project, + version=self.version, + config={'version': 2} + ) + build_three = get( + Build, + project=self.project, + version=self.version, + config={'version': 3}, + success=False, + ) + + self.assertIsNone(build_one.previous) + self.assertEqual(build_two.previous, build_one) + self.assertEqual(build_three.previous, build_two) + self.assertEqual(build_three.previous.previous, build_one) + + def test_normal_save_config(self): + build = get( + Build, + project=self.project, + version=self.version, + ) + build.config = {'version': 1} + build.save() + self.assertEqual(build.config, {'version': 1}) + + build.config = {'version': 2} + build.save() + self.assertEqual(build.config, {'version': 2}) + + def test_save_same_config(self): + build_one = get( + Build, + project=self.project, + version=self.version, + ) + build_one.config = {} + build_one.save() + + build_two = get( + Build, + project=self.project, + version=self.version, + ) + build_two.config = {'version': 2} + build_two.save() + + self.assertEqual(build_two.config, {'version': 2}) + + def test_save_same_config_previous_empty(self): + build_one = get( + Build, + project=self.project, + version=self.version, + ) + build_one.config = {} + build_one.save() + + build_two = get( + Build, + project=self.project, + version=self.version, + ) + build_two.config = {} + build_two.save() + + self.assertEqual(build_two.config, {}) + build_two.config = {'version': 2} + build_two.save() + self.assertEqual(build_two.config, {'version': 2}) + + def test_do_not_save_same_config(self): + build_one = get( + Build, + project=self.project, + version=self.version, + ) + build_one.config = {'version': 1} + build_one.save() + + build_two = get( + Build, + project=self.project, + version=self.version, + ) + build_two.config = {'version': 1} + build_two.save() + self.assertEqual(build_two._config, {Build.CONFIG_KEY: build_one.pk}) + self.assertEqual(build_two.config, {'version': 1}) + + def test_do_not_save_same_config_nested(self): + build_one = get( + Build, + project=self.project, + version=self.version, + ) + build_one.config = {'version': 1} + build_one.save() + + build_two = get( + Build, + project=self.project, + version=self.version, + ) + build_two.config = {'version': 1} + build_two.save() + + build_three = get( + Build, + project=self.project, + version=self.version, + ) + build_three.config = {'version': 1} + build_three.save() + + build_four = get( + Build, + project=self.project, + version=self.version, + ) + build_four.config = {'version': 2} + build_four.save() + + self.assertEqual(build_one.config, {'version': 1}) + self.assertEqual(build_one._config, {'version': 1}) + + self.assertEqual(build_two._config, {Build.CONFIG_KEY: build_one.pk}) + self.assertEqual(build_three._config, {Build.CONFIG_KEY: build_one.pk}) + + self.assertEqual(build_two.config, {'version': 1}) + self.assertEqual(build_three.config, {'version': 1}) + + self.assertEqual(build_four.config, {'version': 2}) + self.assertEqual(build_four._config, {'version': 2}) + + def test_do_not_reference_empty_configs(self): + build_one = get( + Build, + project=self.project, + version=self.version, + ) + build_one.config = {} + build_one.save() + + build_two = get( + Build, + project=self.project, + version=self.version, + ) + build_two.config = {} + build_two.save() + self.assertEqual(build_two._config, {}) + self.assertEqual(build_two.config, {}) diff --git a/readthedocs/rtd_tests/tests/test_version_config.py b/readthedocs/rtd_tests/tests/test_version_config.py new file mode 100644 index 00000000000..82286ade4bf --- /dev/null +++ b/readthedocs/rtd_tests/tests/test_version_config.py @@ -0,0 +1,81 @@ +from __future__ import division, print_function, unicode_literals + +from django.test import TestCase +from django_dynamic_fixture import get + +from readthedocs.builds.models import Build, Version +from readthedocs.projects.models import Project + + +class VersionConfigTests(TestCase): + + def setUp(self): + self.project = get(Project) + self.version = get(Version, project=self.project) + + def test_get_correct_config(self): + build_old = Build.objects.create( + project=self.project, + version=self.version, + config={'version': 1} + ) + build_new = Build.objects.create( + project=self.project, + version=self.version, + config={'version': 2} + ) + build_new_error = Build.objects.create( + project=self.project, + version=self.version, + config={'version': 3}, + success=False, + ) + build_new_unfinish = Build.objects.create( + project=self.project, + version=self.version, + config={'version': 4}, + state='building', + ) + self.assertEqual(self.version.config, {'version': 2}) + + def test_get_correct_config_when_same_config(self): + build_old = get( + Build, + project=self.project, + version=self.version, + config={} + ) + build_old.config = {'version': 1} + build_old.save() + + build_new = get( + Build, + project=self.project, + version=self.version, + config={} + ) + build_new.config = {'version': 1} + build_new.save() + + build_new_error = get( + Build, + project=self.project, + version=self.version, + config={}, + success=False, + ) + build_new_error.config = {'version': 3} + build_new_error.save() + + build_new_unfinish = get( + Build, + project=self.project, + version=self.version, + config={}, + state='building', + ) + build_new_unfinish.config = {'version': 1} + build_new_unfinish.save() + + config = self.version.config + self.assertEqual(config, {'version': 1})