From bc8cee009d32da082927145de8ebc1d8ea6a1370 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 31 Jan 2022 12:48:38 +0100 Subject: [PATCH 1/5] Django3: add new `django.db.models.JSONField` Create new JSON fields postfixed with `_json` for all the JSON fields defined via `jsonfield` third party package. --- .../migrations/0038_add_new_jsonfields.py | 31 ++++++++++ readthedocs/builds/models.py | 5 ++ readthedocs/integrations/admin.py | 1 + .../migrations/0008_add_new_jsonfields.py | 58 +++++++++++++++++++ readthedocs/integrations/models.py | 13 +++++ 5 files changed, 108 insertions(+) create mode 100644 readthedocs/builds/migrations/0038_add_new_jsonfields.py create mode 100644 readthedocs/integrations/migrations/0008_add_new_jsonfields.py diff --git a/readthedocs/builds/migrations/0038_add_new_jsonfields.py b/readthedocs/builds/migrations/0038_add_new_jsonfields.py new file mode 100644 index 00000000000..1451cbc6d94 --- /dev/null +++ b/readthedocs/builds/migrations/0038_add_new_jsonfields.py @@ -0,0 +1,31 @@ +# Generated by Django 3.2.11 on 2022-01-31 11:47 + +from django.db import migrations, models +from django.db.models import F +from django.db.models.functions import Cast + + +def forwards_func(apps, schema_editor): + Build = apps.get_model('builds', 'Build') + ( + Build.objects + .annotate(_config_in_json=Cast('_config', output_field=models.JSONField())) + # Copy `_config` JSONField (text) into `_config_json` (jsonb) + .update(_config_json=F('_config_in_json')) + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ('builds', '0037_alter_build_cold_storage'), + ] + + operations = [ + migrations.AddField( + model_name='build', + name='_config_json', + field=models.JSONField(default=dict, verbose_name='Configuration used in the build'), + ), + migrations.RunPython(forwards_func) + ] diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index dbedd683142..24da472b10d 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -648,6 +648,7 @@ class Build(models.Model): blank=True, ) _config = JSONField(_('Configuration used in the build'), default=dict) + _config_json = models.JSONField(_('Configuration used in the build'), default=dict) length = models.IntegerField(_('Build Length'), null=True, blank=True) @@ -716,6 +717,10 @@ def config(self): Build object (it could be stored in this object or one of the previous ones). """ + # TODO: now that we are using a proper JSONField here, we could + # probably change this field to be a ForeignKey to avoid repeating the + # config file over and over again and re-use them to save db data as + # well if self.CONFIG_KEY in self._config: return ( Build.objects diff --git a/readthedocs/integrations/admin.py b/readthedocs/integrations/admin.py index 0d02eb145de..6d105cf44b8 100644 --- a/readthedocs/integrations/admin.py +++ b/readthedocs/integrations/admin.py @@ -93,6 +93,7 @@ class IntegrationAdmin(admin.ModelAdmin): search_fields = ('project__slug', 'project__name') readonly_fields = ['exchanges'] + # TODO: review this now that we are using official Django's JSONField def exchanges(self, obj): """ Manually make an inline-ish block. diff --git a/readthedocs/integrations/migrations/0008_add_new_jsonfields.py b/readthedocs/integrations/migrations/0008_add_new_jsonfields.py new file mode 100644 index 00000000000..5b3dc10a098 --- /dev/null +++ b/readthedocs/integrations/migrations/0008_add_new_jsonfields.py @@ -0,0 +1,58 @@ +# Generated by Django 3.2.11 on 2022-01-31 11:47 + +from django.db import migrations, models +from django.db.models import F +from django.db.models.functions import Cast + + +def forwards_func(apps, schema_editor): + HttpExchange = apps.get_model('integrations', 'HttpExchange') + ( + HttpExchange.objects + .annotate( + request_headers_in_json=Cast('request_headers', output_field=models.JSONField()), + response_headers_in_json=Cast('response_headers', output_field=models.JSONField()), + ) + .update( + request_headers_json=F('request_headers_in_json'), + response_headers_json=F('response_headers_in_json'), + ) + ) + + + Integration = apps.get_model('integrations', 'Integration') + ( + Integration.objects + .annotate( + provider_data_in_json=Cast('provider_data', output_field=models.JSONField()), + ) + .update( + provider_data_json=F('provider_data_in_json'), + ) + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ('integrations', '0007_update-provider-data'), + ] + + operations = [ + migrations.AddField( + model_name='httpexchange', + name='request_headers_json', + field=models.JSONField(default=None, null=True, verbose_name='Request headers'), + ), + migrations.AddField( + model_name='httpexchange', + name='response_headers_json', + field=models.JSONField(default=None, null=True, verbose_name='Request headers'), + ), + migrations.AddField( + model_name='integration', + name='provider_data_json', + field=models.JSONField(default=dict, verbose_name='Provider data'), + ), + migrations.RunPython(forwards_func) + ] diff --git a/readthedocs/integrations/models.py b/readthedocs/integrations/models.py index 2530df85c14..0a3746dae1b 100644 --- a/readthedocs/integrations/models.py +++ b/readthedocs/integrations/models.py @@ -145,9 +145,21 @@ class HttpExchange(models.Model): date = models.DateTimeField(_('Date'), auto_now_add=True) request_headers = JSONField(_('Request headers')) + request_headers_json = models.JSONField( + _('Request headers'), + # Delete after deploy + null=True, + default=None, + ) request_body = models.TextField(_('Request body')) response_headers = JSONField(_('Request headers')) + response_headers_json = models.JSONField( + _('Request headers'), + # Delete after deploy + null=True, + default=None, + ) response_body = models.TextField(_('Response body')) status_code = models.IntegerField( @@ -278,6 +290,7 @@ class Integration(models.Model): choices=INTEGRATIONS, ) provider_data = JSONField(_('Provider data'), default=dict) + provider_data_json = models.JSONField(_('Provider data'), default=dict) exchanges = GenericRelation( 'HttpExchange', related_query_name='integrations', From db3cfd86fd229567be09006d0331fb13604eff67 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Feb 2022 16:55:04 -0300 Subject: [PATCH 2/5] Migrations: do not loss data while deploying Apply Santos' technique to avoid lossing data while doing the deploy. See https://github.com/readthedocs/readthedocs.org/pull/8868#issuecomment-1029243541 --- .../migrations/0038_add_new_jsonfields.py | 11 ----- .../migrations/0039_migrate_config_data.py | 24 +++++++++++ readthedocs/builds/models.py | 5 +++ .../migrations/0008_add_new_jsonfields.py | 28 ------------- .../migrations/0009_migrate_headers_data.py | 42 +++++++++++++++++++ readthedocs/integrations/models.py | 15 +++++++ 6 files changed, 86 insertions(+), 39 deletions(-) create mode 100644 readthedocs/builds/migrations/0039_migrate_config_data.py create mode 100644 readthedocs/integrations/migrations/0009_migrate_headers_data.py diff --git a/readthedocs/builds/migrations/0038_add_new_jsonfields.py b/readthedocs/builds/migrations/0038_add_new_jsonfields.py index 1451cbc6d94..0a7b7a0bc41 100644 --- a/readthedocs/builds/migrations/0038_add_new_jsonfields.py +++ b/readthedocs/builds/migrations/0038_add_new_jsonfields.py @@ -5,16 +5,6 @@ from django.db.models.functions import Cast -def forwards_func(apps, schema_editor): - Build = apps.get_model('builds', 'Build') - ( - Build.objects - .annotate(_config_in_json=Cast('_config', output_field=models.JSONField())) - # Copy `_config` JSONField (text) into `_config_json` (jsonb) - .update(_config_json=F('_config_in_json')) - ) - - class Migration(migrations.Migration): dependencies = [ @@ -27,5 +17,4 @@ class Migration(migrations.Migration): name='_config_json', field=models.JSONField(default=dict, verbose_name='Configuration used in the build'), ), - migrations.RunPython(forwards_func) ] diff --git a/readthedocs/builds/migrations/0039_migrate_config_data.py b/readthedocs/builds/migrations/0039_migrate_config_data.py new file mode 100644 index 00000000000..bfbd0fb58f6 --- /dev/null +++ b/readthedocs/builds/migrations/0039_migrate_config_data.py @@ -0,0 +1,24 @@ +from django.db import migrations, models +from django.db.models import F +from django.db.models.functions import Cast + + +def forwards_func(apps, schema_editor): + Build = apps.get_model('builds', 'Build') + ( + Build.objects + .annotate(_config_in_json=Cast('_config', output_field=models.JSONField())) + # Copy `_config` JSONField (text) into `_config_json` (jsonb) + .update(_config_json=F('_config_in_json')) + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ('builds', '0038_add_new_jsonfields'), + ] + + operations = [ + migrations.RunPython(forwards_func) + ] diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 24da472b10d..49fa3472b0e 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -764,6 +764,11 @@ def save(self, *args, **kwargs): # noqa self.version_name = self.version.verbose_name self.version_slug = self.version.slug self.version_type = self.version.type + + # TODO: delete copying config after deploy + # Copy `_config` into the new `_config_json` JSONField + self._config_json = self._config + super().save(*args, **kwargs) self._config_changed = False diff --git a/readthedocs/integrations/migrations/0008_add_new_jsonfields.py b/readthedocs/integrations/migrations/0008_add_new_jsonfields.py index 5b3dc10a098..7ba536c348a 100644 --- a/readthedocs/integrations/migrations/0008_add_new_jsonfields.py +++ b/readthedocs/integrations/migrations/0008_add_new_jsonfields.py @@ -5,33 +5,6 @@ from django.db.models.functions import Cast -def forwards_func(apps, schema_editor): - HttpExchange = apps.get_model('integrations', 'HttpExchange') - ( - HttpExchange.objects - .annotate( - request_headers_in_json=Cast('request_headers', output_field=models.JSONField()), - response_headers_in_json=Cast('response_headers', output_field=models.JSONField()), - ) - .update( - request_headers_json=F('request_headers_in_json'), - response_headers_json=F('response_headers_in_json'), - ) - ) - - - Integration = apps.get_model('integrations', 'Integration') - ( - Integration.objects - .annotate( - provider_data_in_json=Cast('provider_data', output_field=models.JSONField()), - ) - .update( - provider_data_json=F('provider_data_in_json'), - ) - ) - - class Migration(migrations.Migration): dependencies = [ @@ -54,5 +27,4 @@ class Migration(migrations.Migration): name='provider_data_json', field=models.JSONField(default=dict, verbose_name='Provider data'), ), - migrations.RunPython(forwards_func) ] diff --git a/readthedocs/integrations/migrations/0009_migrate_headers_data.py b/readthedocs/integrations/migrations/0009_migrate_headers_data.py new file mode 100644 index 00000000000..e9f544b444b --- /dev/null +++ b/readthedocs/integrations/migrations/0009_migrate_headers_data.py @@ -0,0 +1,42 @@ +from django.db import migrations, models +from django.db.models import F +from django.db.models.functions import Cast + + +def forwards_func(apps, schema_editor): + HttpExchange = apps.get_model('integrations', 'HttpExchange') + ( + HttpExchange.objects + .annotate( + request_headers_in_json=Cast('request_headers', output_field=models.JSONField()), + response_headers_in_json=Cast('response_headers', output_field=models.JSONField()), + ) + .update( + request_headers_json=F('request_headers_in_json'), + response_headers_json=F('response_headers_in_json'), + ) + ) + + + Integration = apps.get_model('integrations', 'Integration') + ( + Integration.objects + .annotate( + provider_data_in_json=Cast('provider_data', output_field=models.JSONField()), + ) + .update( + provider_data_json=F('provider_data_in_json'), + ) + ) + + + +class Migration(migrations.Migration): + + dependencies = [ + ('integrations', '0008_add_new_jsonfields'), + ] + + operations = [ + migrations.RunPython(forwards_func) + ] diff --git a/readthedocs/integrations/models.py b/readthedocs/integrations/models.py index 0a3746dae1b..0b00faf6c35 100644 --- a/readthedocs/integrations/models.py +++ b/readthedocs/integrations/models.py @@ -175,6 +175,14 @@ class Meta: def __str__(self): return _('Exchange {0}').format(self.pk) + + # TODO: delete .save method after deploy + # Copy headers into new JSONField + def save(self, *args, **kwargs): + self.request_headers_json = self.request_headers + self.response_headers_json = self.response_headers + super().save(*args, **kwargs) + @property def failed(self): # Assume anything that isn't 2xx level status code is an error @@ -316,6 +324,13 @@ def remove_secret(self): self.secret = None self.save(update_fields=['secret']) + # TODO: delete .save method after deploy + # Copy `provider_data` into new JSONField + def save(self, *args, **kwargs): + self.provider_data_json = self.provider_data + super().save(*args, **kwargs) + + def __str__(self): return ( _('{0} for {1}') From f80ac718478a8806922b18cd0b5de3ec70440fbc Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 3 Feb 2022 22:04:11 +0100 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Santos Gallegos --- readthedocs/builds/migrations/0038_add_new_jsonfields.py | 2 -- readthedocs/integrations/migrations/0008_add_new_jsonfields.py | 2 -- 2 files changed, 4 deletions(-) diff --git a/readthedocs/builds/migrations/0038_add_new_jsonfields.py b/readthedocs/builds/migrations/0038_add_new_jsonfields.py index 0a7b7a0bc41..6dc7923a9f4 100644 --- a/readthedocs/builds/migrations/0038_add_new_jsonfields.py +++ b/readthedocs/builds/migrations/0038_add_new_jsonfields.py @@ -1,8 +1,6 @@ # Generated by Django 3.2.11 on 2022-01-31 11:47 from django.db import migrations, models -from django.db.models import F -from django.db.models.functions import Cast class Migration(migrations.Migration): diff --git a/readthedocs/integrations/migrations/0008_add_new_jsonfields.py b/readthedocs/integrations/migrations/0008_add_new_jsonfields.py index 7ba536c348a..99fd29cc209 100644 --- a/readthedocs/integrations/migrations/0008_add_new_jsonfields.py +++ b/readthedocs/integrations/migrations/0008_add_new_jsonfields.py @@ -1,8 +1,6 @@ # Generated by Django 3.2.11 on 2022-01-31 11:47 from django.db import migrations, models -from django.db.models import F -from django.db.models.functions import Cast class Migration(migrations.Migration): From 0da786e3ce8ca181f8052a75b98429787047326b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 14 Feb 2022 13:10:08 -0300 Subject: [PATCH 4/5] Test: fix tests to work with real JSON fields - cast dict to make them serializable - explicitly declare the fields used for IntegrationForm model - use the default empty dict instead of `None` since it's not allowed in JSON fields --- readthedocs/integrations/models.py | 8 ++++---- readthedocs/projects/forms.py | 5 ++++- readthedocs/rtd_tests/tests/test_oauth.py | 6 +++--- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/readthedocs/integrations/models.py b/readthedocs/integrations/models.py index 0b00faf6c35..f2253fa68ff 100644 --- a/readthedocs/integrations/models.py +++ b/readthedocs/integrations/models.py @@ -175,12 +175,13 @@ class Meta: def __str__(self): return _('Exchange {0}').format(self.pk) - # TODO: delete .save method after deploy # Copy headers into new JSONField def save(self, *args, **kwargs): - self.request_headers_json = self.request_headers - self.response_headers_json = self.response_headers + # NOTE: cast headers into a regular dict because Django does not know + # how to serialize ``requests.structures.CaseInsensitiveDict`` + self.request_headers_json = dict(self.request_headers) + self.response_headers_json = dict(self.response_headers) super().save(*args, **kwargs) @property @@ -330,7 +331,6 @@ def save(self, *args, **kwargs): self.provider_data_json = self.provider_data super().save(*args, **kwargs) - def __str__(self): return ( _('{0} for {1}') diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 3053f265249..a2d2bd087f9 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -682,7 +682,10 @@ class IntegrationForm(forms.ModelForm): class Meta: model = Integration - exclude = ['provider_data', 'exchanges', 'secret'] # pylint: disable=modelform-uses-exclude + fields = [ + 'project', + 'integration_type', + ] def __init__(self, *args, **kwargs): self.project = kwargs.pop('project', None) diff --git a/readthedocs/rtd_tests/tests/test_oauth.py b/readthedocs/rtd_tests/tests/test_oauth.py index 57e9f6197d3..2e246423d3a 100644 --- a/readthedocs/rtd_tests/tests/test_oauth.py +++ b/readthedocs/rtd_tests/tests/test_oauth.py @@ -346,7 +346,7 @@ def test_update_webhook_404_error(self, setup_webhook, session): @mock.patch('readthedocs.oauth.services.github.GitHubService.get_session') @mock.patch('readthedocs.oauth.services.github.GitHubService.setup_webhook') def test_update_webhook_no_provider_data(self, setup_webhook, session): - self.integration.provider_data = None + self.integration.provider_data = {} self.integration.save() session().patch.side_effect = AttributeError @@ -794,7 +794,7 @@ def test_update_webhook_404_error(self, setup_webhook, session): @mock.patch('readthedocs.oauth.services.bitbucket.BitbucketService.get_session') @mock.patch('readthedocs.oauth.services.bitbucket.BitbucketService.setup_webhook') def test_update_webhook_no_provider_data(self, setup_webhook, session): - self.integration.provider_data = None + self.integration.provider_data = {} self.integration.save() session().put.side_effect = AttributeError @@ -1279,7 +1279,7 @@ def test_update_webhook_404_error(self, repo_id, setup_webhook, session): @mock.patch('readthedocs.oauth.services.gitlab.GitLabService.setup_webhook') @mock.patch('readthedocs.oauth.services.gitlab.GitLabService._get_repo_id') def test_update_webhook_no_provider_data(self, repo_id, setup_webhook, session): - self.integration.provider_data = None + self.integration.provider_data = {} self.integration.save() repo_id.return_value = '9999' From 0b90cd3444f0901580ae06fbb2ef5a015a29eb9b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 14 Feb 2022 13:23:53 -0300 Subject: [PATCH 5/5] Test: create objects with dict instead of str Instead of using JSON as string, we pass a real dict object. --- readthedocs/rtd_tests/tests/test_privacy_urls.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_privacy_urls.py b/readthedocs/rtd_tests/tests/test_privacy_urls.py index 193c5b72ce8..d769590a389 100644 --- a/readthedocs/rtd_tests/tests/test_privacy_urls.py +++ b/readthedocs/rtd_tests/tests/test_privacy_urls.py @@ -167,8 +167,8 @@ def setUp(self): # For whatever reason, fixtures hates JSONField self.integration_exchange = HttpExchange.objects.create( related_object=self.integration, - request_headers='{"foo": "bar"}', - response_headers='{"foo": "bar"}', + request_headers={'foo': 'bar'}, + response_headers={'foo': 'bar'}, status_code=200, ) self.domain = get(Domain, domain='docs.foobar.com', project=self.pip) @@ -183,8 +183,8 @@ def setUp(self): self.webhook = get(WebHook, project=self.pip) self.webhook_exchange = HttpExchange.objects.create( related_object=self.webhook, - request_headers='{"foo": "bar"}', - response_headers='{"foo": "bar"}', + request_headers={'foo': 'bar'}, + response_headers={'foo': 'bar'}, status_code=200, ) self.default_kwargs = {