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

💥 Make service FKs in API group config models not nullable #4916

Merged
merged 2 commits into from
Dec 17, 2024
Merged
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
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
80 changes: 80 additions & 0 deletions bin/check_api_groups_null.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
#!/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
from openforms.registrations.contrib.zgw_apis.models import ZGWApiGroupConfig

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"))

zgw_groups = ZGWApiGroupConfig.objects.exclude(
zrc_service__isnull=False,
drc_service__isnull=False,
ztc_service__isnull=False,
).values_list(
"id",
"name",
"zrc_service_id",
"drc_service_id",
"ztc_service_id",
)
for pk, name, zrc_service_id, drc_service_id, ztc_service_id in zgw_groups:
problem = ("ZGW APIs", pk, name)
if zrc_service_id is None:
problems.append((*problem, "No Zaken API service configured"))
if drc_service_id is None:
problems.append((*problem, "No Documenten API service configured"))
if ztc_service_id is None:
problems.append((*problem, "No Catalogi API 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."
)
print(
tabulate(
problems,
headers=("API group type", "ID", "Name", "Problem"),
)
)
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a major thing, but does this have to return something?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, the return value is checked by the upgrade check which blocks migrating if problems are detected! See

for script in self.scripts:



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
16 changes: 0 additions & 16 deletions src/openforms/prefill/contrib/objects_api/tests/test_config.py
Original file line number Diff line number Diff line change
@@ -1,9 +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
from zgw_consumers.constants import APITypes, AuthTypes
from zgw_consumers.test.factories import ServiceFactory
Expand Down Expand Up @@ -41,19 +38,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
Loading
Loading