Skip to content

Commit

Permalink
💥 [#3283] Make objects/objecttypes FKs non-nullable in Objects API group
Browse files Browse the repository at this point in the history
Catalogi and Documents API are optional, but the objecttypes and objects
API services are not. These fields were null because the model was
originally a django-solo singleton model which must be able to be
created without any services existing prior.

A check script is added to prevent being stuck in half-migrated
upgrades.
  • Loading branch information
sergei-maertens committed Dec 12, 2024
1 parent 3a70208 commit 0b4513f
Show file tree
Hide file tree
Showing 13 changed files with 133 additions and 110 deletions.
1 change: 1 addition & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ COPY \
./bin/check_celery_worker_liveness.py \
./bin/report_component_problems.py \
./bin/check_temporary_uploads.py \
./bin/check_api_groups_null.py \
./bin/fix_selectboxes_component_default_values.py \
./bin/

Expand Down
56 changes: 56 additions & 0 deletions bin/check_api_groups_null.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#!/usr/bin/env python
import sys
from pathlib import Path

import django

from tabulate import tabulate

SRC_DIR = Path(__file__).parent.parent / "src"
sys.path.insert(0, str(SRC_DIR.resolve()))


def check_for_null_services_in_api_groups():
from openforms.contrib.objects_api.models import ObjectsAPIGroupConfig

problems: list[tuple[str, int, str, str]] = []

objects_groups = ObjectsAPIGroupConfig.objects.exclude(
objects_service__isnull=False, objecttypes_service__isnull=False
).values_list("id", "name", "objects_service_id", "objecttypes_service_id")

for pk, name, objects_service_id, objecttypes_service_id in objects_groups:
problem = ("Objects API", pk, name)
if objects_service_id is None:
problems.append((*problem, "No objects service configured"))
if objecttypes_service_id is None:
problems.append((*problem, "No object types service configured"))

if not problems:
return False

print(
"Can't upgrade yet - some API group services are not properly configured yet."
)
print(
"Go into the admin to fix their configuration, and then try to upgrade again."
)
tabulate(
problems,
headers=("API group type", "ID", "Name", "Problem"),
)
return True


def main(skip_setup=False) -> bool:
from openforms.setup import setup_env

if not skip_setup:
setup_env()
django.setup()

return check_for_null_services_in_api_groups()


if __name__ == "__main__":
main()
18 changes: 10 additions & 8 deletions src/openforms/contrib/objects_api/clients/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
in the form builder
"""

from __future__ import annotations

from typing import TYPE_CHECKING

from zgw_consumers.client import build_client
Expand All @@ -26,25 +28,25 @@ class NoServiceConfigured(RuntimeError):
pass


def get_objects_client(config: "ObjectsAPIGroupConfig") -> ObjectsClient:
if not (service := config.objects_service):
raise NoServiceConfigured("No Objects API service configured!")
def get_objects_client(config: ObjectsAPIGroupConfig) -> ObjectsClient:
service = config.objects_service
assert service is not None
return build_client(service, client_factory=ObjectsClient)


def get_objecttypes_client(config: "ObjectsAPIGroupConfig") -> ObjecttypesClient:
if not (service := config.objecttypes_service):
raise NoServiceConfigured("No Objecttypes API service configured!")
def get_objecttypes_client(config: ObjectsAPIGroupConfig) -> ObjecttypesClient:
service = config.objecttypes_service
assert service is not None
return build_client(service, client_factory=ObjecttypesClient)


def get_documents_client(config: "ObjectsAPIGroupConfig") -> DocumentenClient:
def get_documents_client(config: ObjectsAPIGroupConfig) -> DocumentenClient:
if not (service := config.drc_service):
raise NoServiceConfigured("No Documents API service configured!")
return build_client(service, client_factory=DocumentenClient)


def get_catalogi_client(config: "ObjectsAPIGroupConfig") -> CatalogiClient:
def get_catalogi_client(config: ObjectsAPIGroupConfig) -> CatalogiClient:
if not (service := config.catalogi_service):
raise NoServiceConfigured("No Catalogi API service configured!")
return build_client(service, client_factory=CatalogiClient)
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Generated by Django 4.2.17 on 2024-12-12 22:40

import django.db.models.deletion
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("zgw_consumers", "0022_set_default_service_slug"),
("objects_api", "0003_objectsapigroupconfig_identifier_unique"),
]

operations = [
migrations.AlterField(
model_name="objectsapigroupconfig",
name="objects_service",
field=models.ForeignKey(
limit_choices_to={"api_type": "orc"},
on_delete=django.db.models.deletion.PROTECT,
related_name="+",
to="zgw_consumers.service",
verbose_name="Objects API",
),
),
migrations.AlterField(
model_name="objectsapigroupconfig",
name="objecttypes_service",
field=models.ForeignKey(
limit_choices_to={"api_type": "orc"},
on_delete=django.db.models.deletion.PROTECT,
related_name="+",
to="zgw_consumers.service",
verbose_name="Objecttypes API",
),
),
]
5 changes: 2 additions & 3 deletions src/openforms/contrib/objects_api/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@


class ObjectsAPIGroupConfig(models.Model):
# TODO OF 3.0: remove `null=True` from the service fields
name = models.CharField(
_("name"),
max_length=255,
Expand All @@ -30,15 +29,15 @@ class ObjectsAPIGroupConfig(models.Model):
verbose_name=_("Objects API"),
on_delete=models.PROTECT,
limit_choices_to={"api_type": APITypes.orc},
null=True,
null=False,
related_name="+",
)
objecttypes_service = models.ForeignKey(
"zgw_consumers.Service",
verbose_name=_("Objecttypes API"),
on_delete=models.PROTECT,
limit_choices_to={"api_type": APITypes.orc},
null=True,
null=False,
related_name="+",
)
drc_service = models.ForeignKey(
Expand Down
3 changes: 1 addition & 2 deletions src/openforms/contrib/objects_api/tests/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
from openforms.accounts.tests.factories import SuperUserFactory
from openforms.utils.tests.vcr import OFVCRMixin

from ..models import ObjectsAPIGroupConfig
from .factories import ObjectsAPIGroupConfigFactory

TEST_FILES = Path(__file__).parent / "files"
Expand All @@ -20,7 +19,7 @@ class ObjectsAPIGroupConfigAdminTest(OFVCRMixin, WebTest):
VCR_TEST_FILES = TEST_FILES

def test_name(self):
ObjectsAPIGroupConfig.objects.create(name="test group")
ObjectsAPIGroupConfigFactory.create(name="test group")
user = SuperUserFactory.create()

response = self.app.get(
Expand Down
14 changes: 0 additions & 14 deletions src/openforms/prefill/contrib/objects_api/tests/test_config.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from pathlib import Path
from unittest.mock import patch

from django.test import override_settings
from django.utils.translation import gettext as _

from rest_framework.test import APITestCase
Expand Down Expand Up @@ -41,19 +40,6 @@ def setUp(self):
for_test_docker_compose=True
)

@override_settings(LANGUAGE_CODE="en")
def test_undefined_service_raises_exception(self):
self.objects_api_group.objects_service = None
self.objects_api_group.save()

with self.assertRaisesMessage(
InvalidPluginConfiguration,
_(
"Objects API endpoint is not configured for Objects API group {objects_api_group}."
).format(objects_api_group=self.objects_api_group),
):
plugin.check_config()

def test_invalid_service_raises_exception(self):
objects_service = ServiceFactory.create(
api_root="http://localhost:8002/api/v2/invalid",
Expand Down
10 changes: 4 additions & 6 deletions src/openforms/registrations/contrib/objects_api/config.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import warnings

from django.db.models import IntegerChoices, Q
from django.db.models import IntegerChoices
from django.utils.translation import gettext_lazy as _

from drf_spectacular.utils import OpenApiExample, extend_schema_serializer
Expand Down Expand Up @@ -67,11 +67,9 @@ class ObjecttypeVariableMappingSerializer(serializers.Serializer):

class ObjectsAPIOptionsSerializer(JsonSchemaSerializerMixin, serializers.Serializer):
objects_api_group = PrimaryKeyRelatedAsChoicesField(
queryset=ObjectsAPIGroupConfig.objects.exclude(
Q(objects_service=None)
| Q(objecttypes_service=None)
| Q(drc_service=None)
| Q(catalogi_service=None)
queryset=ObjectsAPIGroupConfig.objects.filter(
drc_service__isnull=False,
catalogi_service__isnull=False,
),
label=("Objects API group"),
help_text=_("Which Objects API group to use."),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,16 @@
from rest_framework.reverse import reverse_lazy
from rest_framework.test import APITestCase
from zgw_consumers.constants import APITypes, AuthTypes
from zgw_consumers.models import Service
from zgw_consumers.test.factories import ServiceFactory

from openforms.accounts.tests.factories import StaffUserFactory, UserFactory
from openforms.contrib.objects_api.tests.factories import ObjectsAPIGroupConfigFactory
from openforms.utils.tests.feature_flags import enable_feature_flag
from openforms.utils.tests.vcr import OFVCRMixin

from ..models import ObjectsAPIGroupConfig

TEST_FILES = Path(__file__).parent / "files"


def get_test_config() -> ObjectsAPIGroupConfig:
"""Returns a preconfigured ``ObjectsAPIGroupConfig`` instance matching the docker compose configuration."""

return ObjectsAPIGroupConfig(
objecttypes_service=Service(
api_root="http://localhost:8001/api/v2/",
api_type=APITypes.orc,
oas="https://example.com/",
header_key="Authorization",
header_value="Token 171be5abaf41e7856b423ad513df1ef8f867ff48",
auth_type=AuthTypes.api_key,
)
)


class ObjecttypesAPIEndpointTests(OFVCRMixin, APITestCase):

VCR_TEST_FILES = TEST_FILES
Expand All @@ -40,9 +22,8 @@ class ObjecttypesAPIEndpointTests(OFVCRMixin, APITestCase):
@classmethod
def setUpTestData(cls) -> None:
super().setUpTestData()
cls.config = get_test_config()
cls.config.objecttypes_service.save()
cls.config.save()

cls.config = ObjectsAPIGroupConfigFactory.create(for_test_docker_compose=True)

def test_auth_required(self):
response = self.client.get(self.endpoint)
Expand Down Expand Up @@ -101,9 +82,8 @@ class ObjecttypeVersionsAPIEndpointTests(OFVCRMixin, APITestCase):
@classmethod
def setUpTestData(cls) -> None:
super().setUpTestData()
cls.config = get_test_config()
cls.config.objecttypes_service.save()
cls.config.save()

cls.config = ObjectsAPIGroupConfigFactory.create(for_test_docker_compose=True)

def test_auth_required(self):
response = self.client.get(self.endpoint)
Expand Down Expand Up @@ -156,9 +136,8 @@ class TargetPathsAPIEndpointTests(OFVCRMixin, APITestCase):
@classmethod
def setUpTestData(cls) -> None:
super().setUpTestData()
cls.config = get_test_config()
cls.config.objecttypes_service.save()
cls.config.save()

cls.config = ObjectsAPIGroupConfigFactory.create(for_test_docker_compose=True)

def test_auth_required(self):
response = self.client.post(self.endpoint)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,28 +40,6 @@ def _mockForValidServiceConfiguration(self, m: requests_mock.Mocker) -> None:
headers={"API-version": "1.0.0"},
)

def test_no_objects_service_configured(self):
self.config.objects_service = None
self.config.save()
plugin = ObjectsAPIRegistration(PLUGIN_IDENTIFIER)

with self.assertRaises(InvalidPluginConfiguration):
plugin.check_config()

@requests_mock.Mocker()
def test_no_objecttypes_service_configured(self, m: requests_mock.Mocker):
# Objects API needs to be set up as services are checked in a certain order
m.get(
"https://objects.example.com/api/v1/objects?pageSize=1",
json={"results": []},
)
self.config.objecttypes_service = None
self.config.save()
plugin = ObjectsAPIRegistration(PLUGIN_IDENTIFIER)

with self.assertRaises(InvalidPluginConfiguration):
plugin.check_config()

@requests_mock.Mocker()
def test_objects_service_misconfigured_connection_error(self, m):
m.get(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,6 @@ def setUpTestData(cls) -> None:
for_test_docker_compose=True
)

# This group shouldn't be usable:
cls.invalid_objects_api_group = ObjectsAPIGroupConfigFactory.create(
objecttypes_service=None,
drc_service=None,
catalogi_service=None,
)

def test_invalid_fields_v1(self):
options = ObjectsAPIOptionsSerializer(
data={
Expand Down Expand Up @@ -170,24 +163,6 @@ def test_unknown_objecttype_version(self):
error = options.errors["objecttype_version"][0]
self.assertEqual(error.code, "not-found")

def test_invalid_objects_api_group(self):
options = ObjectsAPIOptionsSerializer(
data={
"objects_api_group": self.invalid_objects_api_group.pk,
"version": 2,
"objecttype": "8e46e0a5-b1b4-449b-b9e9-fa3cea655f48",
"objecttype_version": 1,
"informatieobjecttype_attachment": "http://localhost:8003/catalogi/api/v1/informatieobjecttypen/7a474713-0833-402a-8441-e467c08ac55b",
"informatieobjecttype_submission_report": "http://localhost:8003/catalogi/api/v1/informatieobjecttypen/b2d83b94-9b9b-4e80-a82f-73ff993c62f3",
"informatieobjecttype_submission_csv": "http://localhost:8003/catalogi/api/v1/informatieobjecttypen/531f6c1a-97f7-478c-85f0-67d2f23661c7",
},
)

self.assertFalse(options.is_valid())
self.assertIn("objects_api_group", options.errors)
error = options.errors["objects_api_group"][0]
self.assertEqual(error.code, "does_not_exist")

def test_auth_attribute_path_required_if_update_existing_object_is_true(self):
options = ObjectsAPIOptionsSerializer(
data={
Expand Down
Loading

0 comments on commit 0b4513f

Please sign in to comment.