refactor: enhance db existence validation (#1350)

Some refactoring in validators. Some of them are not used anymore or
they do check for the existence of the DB by fetching the record, which
is inefficient. DRF validators are expected to raise `ValidationError`
or return None. In case of the `organization_id` has been created a
mixin to avoid code duplication.

Refactored:
- organization
- rulebook
- credential_type
- awx_token

Removed:
- event_streams
- extra_vars

Technically, in most of the cases a PrimaryKey field would be better,
but that would require a more extended refactor to keep 100% backward
compatibility, since we would have to implement custom error messages
among other changes. There are multiple fields candidate to be a
PrimaryKeyField and such a conversion is not the scope of this PR.
Here I only address the fields where we are duplicating DB queries,
duplicating field definition and removing unused validators.

Signed-off-by: Alex <[email protected]>

Author: Alex-Izquierdo

src/aap_eda/api/serializers/activation.py

@@ -35,6 +35,7 @@
 from aap_eda.api.serializers.event_stream import EventStreamOutSerializer
 from aap_eda.api.serializers.fields.basic_user import BasicUserFieldSerializer
 from aap_eda.api.serializers.fields.yaml import YAMLSerializerField
+from aap_eda.api.serializers.mixins import OrganizationIdFieldMixin
 from aap_eda.api.serializers.organization import OrganizationRefSerializer
 from aap_eda.api.serializers.project import (
     ANSIBLE_VAULT_STRING,
@@ -429,7 +430,9 @@ def to_representation(self, activation):
         }
 
 
-class ActivationCreateSerializer(serializers.ModelSerializer):
+class ActivationCreateSerializer(
+    OrganizationIdFieldMixin, serializers.ModelSerializer
+):
     """Serializer for creating the Activation."""
 
     class Meta:
@@ -452,15 +455,6 @@ class Meta:
             "skip_audit_events",
         ]
 
-    organization_id = serializers.IntegerField(
-        required=True,
-        allow_null=False,
-        validators=[validators.check_if_organization_exists],
-        error_messages={
-            "null": "Organization is needed",
-            "required": "Organization is required",
-        },
-    )
     rulebook_id = serializers.IntegerField(
         validators=[validators.check_if_rulebook_exists],
         error_messages={
@@ -597,7 +591,9 @@ def copy(self) -> dict:
         return super().create(copied_data)
 
 
-class ActivationUpdateSerializer(serializers.ModelSerializer):
+class ActivationUpdateSerializer(
+    OrganizationIdFieldMixin, serializers.ModelSerializer
+):
     """Serializer for updating the Activation."""
 
     class Meta:
@@ -620,12 +616,6 @@ class Meta:
             "skip_audit_events",
         ]
 
-    organization_id = serializers.IntegerField(
-        required=False,
-        allow_null=False,
-        validators=[validators.check_if_organization_exists],
-        error_messages={"null": "Organization is needed"},
-    )
     rulebook_id = serializers.IntegerField(
         validators=[validators.check_if_rulebook_exists],
         error_messages={"null": "Rulebook is needed"},

src/aap_eda/api/serializers/decision_environment.py

@@ -16,6 +16,7 @@
 
 from aap_eda.api.serializers.eda_credential import EdaCredentialRefSerializer
 from aap_eda.api.serializers.fields.basic_user import BasicUserFieldSerializer
+from aap_eda.api.serializers.mixins import OrganizationIdFieldMixin
 from aap_eda.api.serializers.organization import OrganizationRefSerializer
 from aap_eda.api.serializers.user import BasicUserSerializer
 from aap_eda.core import models, validators
@@ -56,15 +57,11 @@ def to_representation(self, decision_environment):
         return result
 
 
-class DecisionEnvironmentCreateSerializer(serializers.ModelSerializer):
+class DecisionEnvironmentCreateSerializer(
+    OrganizationIdFieldMixin, serializers.ModelSerializer
+):
     """Serializer for creating the DecisionEnvironment."""
 
-    organization_id = serializers.IntegerField(
-        required=True,
-        allow_null=False,
-        validators=[validators.check_if_organization_exists],
-        error_messages={"null": "Organization is needed"},
-    )
     eda_credential_id = serializers.IntegerField(
         required=False,
         allow_null=True,

src/aap_eda/api/serializers/eda_credential.py

@@ -18,6 +18,7 @@
 
 from aap_eda.api.serializers.credential_type import CredentialTypeRefSerializer
 from aap_eda.api.serializers.fields.basic_user import BasicUserFieldSerializer
+from aap_eda.api.serializers.mixins import OrganizationIdFieldMixin
 from aap_eda.api.serializers.organization import OrganizationRefSerializer
 from aap_eda.api.serializers.user import BasicUserSerializer
 from aap_eda.core import enums, models, validators
@@ -129,7 +130,9 @@ class Meta:
         ]
 
 
-class EdaCredentialCreateSerializer(serializers.ModelSerializer):
+class EdaCredentialCreateSerializer(
+    OrganizationIdFieldMixin, serializers.ModelSerializer
+):
     credential_type_id = serializers.IntegerField(
         required=True,
         allow_null=False,
@@ -139,16 +142,6 @@ class EdaCredentialCreateSerializer(serializers.ModelSerializer):
             "required": "Credential Type is required",
         },
     )
-    organization_id = serializers.IntegerField(
-        required=True,
-        allow_null=False,
-        validators=[validators.check_if_organization_exists],
-        error_messages={
-            "null": "Organization is needed",
-            "required": "Organization is required",
-        },
-    )
-
     inputs = serializers.JSONField()
 
     def validate(self, data):
@@ -190,13 +183,9 @@ class Meta:
         ]
 
 
-class EdaCredentialUpdateSerializer(serializers.ModelSerializer):
-    organization_id = serializers.IntegerField(
-        required=True,
-        allow_null=False,
-        validators=[validators.check_if_organization_exists],
-        error_messages={"null": "Organization is needed"},
-    )
+class EdaCredentialUpdateSerializer(
+    OrganizationIdFieldMixin, serializers.ModelSerializer
+):
     inputs = serializers.JSONField()
 
     def validate(self, data):

src/aap_eda/api/serializers/mixins.py

@@ -13,8 +13,10 @@
 #  limitations under the License.
 
 from django.conf import settings
+from rest_framework import serializers
 
 from aap_eda.api import exceptions as api_exc
+from aap_eda.core import validators
 
 
 class SharedResourceSerializerMixin:
@@ -35,3 +37,15 @@ def validate_shared_resource(self, data=None):
             raise api_exc.Forbidden(
                 f"{action} should be done through the platform ingress"
             )
+
+
+class OrganizationIdFieldMixin(serializers.Serializer):
+    organization_id = serializers.IntegerField(
+        required=True,
+        allow_null=False,
+        validators=[validators.check_if_organization_exists],
+        error_messages={
+            "null": "Organization is needed",
+            "required": "Organization is required",
+        },
+    )

src/aap_eda/api/serializers/project.py

@@ -19,6 +19,7 @@
 
 from aap_eda.api.serializers.eda_credential import EdaCredentialRefSerializer
 from aap_eda.api.serializers.fields.basic_user import BasicUserFieldSerializer
+from aap_eda.api.serializers.mixins import OrganizationIdFieldMixin
 from aap_eda.api.serializers.organization import OrganizationRefSerializer
 from aap_eda.api.serializers.user import BasicUserSerializer
 from aap_eda.core import models, validators
@@ -81,16 +82,9 @@ def to_representation(self, instance):
         return result
 
 
-class ProjectCreateRequestSerializer(serializers.ModelSerializer):
-    organization_id = serializers.IntegerField(
-        required=True,
-        allow_null=False,
-        validators=[validators.check_if_organization_exists],
-        error_messages={
-            "null": "Organization is needed",
-            "required": "Organization is required",
-        },
-    )
+class ProjectCreateRequestSerializer(
+    OrganizationIdFieldMixin, serializers.ModelSerializer
+):
     eda_credential_id = serializers.IntegerField(
         required=False,
         allow_null=True,
@@ -142,16 +136,9 @@ class Meta:
         ]
 
 
-class ProjectUpdateRequestSerializer(serializers.ModelSerializer):
-    organization_id = serializers.IntegerField(
-        required=True,
-        allow_null=False,
-        validators=[validators.check_if_organization_exists],
-        error_messages={
-            "null": "Organization is needed",
-            "required": "Organization is required",
-        },
-    )
+class ProjectUpdateRequestSerializer(
+    OrganizationIdFieldMixin, serializers.ModelSerializer
+):
     name = serializers.CharField(
         required=False,
         allow_blank=False,

src/aap_eda/api/serializers/team.py

@@ -14,8 +14,9 @@
 from rest_framework import serializers
 from rest_framework.validators import UniqueTogetherValidator
 
+from aap_eda.api.serializers.mixins import OrganizationIdFieldMixin
 from aap_eda.api.serializers.organization import OrganizationRefSerializer
-from aap_eda.core import models, validators
+from aap_eda.core import models
 
 from .fields.ansible_resource import AnsibleResourceFieldSerializer
 from .mixins import SharedResourceSerializerMixin
@@ -40,16 +41,10 @@ class Meta:
 
 
 class TeamCreateSerializer(
+    OrganizationIdFieldMixin,
     serializers.ModelSerializer,
     SharedResourceSerializerMixin,
 ):
-    organization_id = serializers.IntegerField(
-        required=True,
-        allow_null=False,
-        validators=[validators.check_if_organization_exists],
-        error_messages={"null": "Organization is needed"},
-    )
-
     class Meta:
         model = models.Team
         fields = [

src/aap_eda/core/validators.py

@@ -43,14 +43,11 @@
 ]
 
 
-def check_if_rulebook_exists(rulebook_id: int) -> int:
-    try:
-        models.Rulebook.objects.get(pk=rulebook_id)
-    except models.Rulebook.DoesNotExist:
+def check_if_rulebook_exists(rulebook_id: int) -> None:
+    if not models.Rulebook.objects.filter(pk=rulebook_id).exists():
         raise serializers.ValidationError(
             f"Rulebook with id {rulebook_id} does not exist"
         )
-    return rulebook_id
 
 
 def check_if_de_exists(decision_environment_id: int) -> int:
@@ -239,14 +236,13 @@ def check_single_aap_credential(
     return eda_credential_ids
 
 
-def check_if_credential_type_exists(credential_type_id: int) -> int:
-    try:
-        models.CredentialType.objects.get(pk=credential_type_id)
-    except models.CredentialType.DoesNotExist:
+def check_if_credential_type_exists(credential_type_id: int) -> None:
+    if not models.CredentialType.objects.filter(
+        pk=credential_type_id
+    ).exists():
         raise serializers.ValidationError(
             f"CredentialType with id {credential_type_id} does not exist"
         )
-    return credential_type_id
 
 
 def check_if_credential_name_used(name: str) -> str:
@@ -260,33 +256,17 @@ def check_if_credential_name_used(name: str) -> str:
 
 
 def check_if_organization_exists(organization_id: int) -> int:
-    try:
-        models.Organization.objects.get(pk=organization_id)
-    except models.Organization.DoesNotExist:
+    if not models.Organization.objects.filter(pk=organization_id).exists():
         raise serializers.ValidationError(
             f"Organization with id {organization_id} does not exist"
         )
-    return organization_id
 
 
-def check_if_extra_var_exists(extra_var_id: int) -> int:
-    try:
-        models.ExtraVar.objects.get(pk=extra_var_id)
-    except models.ExtraVar.DoesNotExist:
-        raise serializers.ValidationError(
-            f"ExtraVar with id {extra_var_id} does not exist"
-        )
-    return extra_var_id
-
-
-def check_if_awx_token_exists(awx_token_id: int) -> int:
-    try:
-        models.AwxToken.objects.get(pk=awx_token_id)
-    except models.AwxToken.DoesNotExist:
+def check_if_awx_token_exists(awx_token_id: int) -> None:
+    if not models.AwxToken.objects.filter(pk=awx_token_id).exists():
         raise serializers.ValidationError(
             f"AwxToken with id {awx_token_id} does not exist"
         )
-    return awx_token_id
 
 
 def check_rulesets_require_token(
@@ -436,18 +416,6 @@ def _validate_event_stream_settings(auth_type: str):
         )
 
 
-def check_if_event_streams_exists(event_stream_ids: list[int]) -> list[int]:
-    """Check a event stream exists."""
-    for event_stream_id in event_stream_ids:
-        try:
-            models.EventStream.objects.get(pk=event_stream_id)
-        except models.EventStream.DoesNotExist as exc:
-            raise serializers.ValidationError(
-                f"EventStream with id {event_stream_id} does not exist"
-            ) from exc
-    return event_stream_ids
-
-
 def check_credential_types_for_event_stream(eda_credential_id: int) -> int:
     """Check the credential types for a event stream."""
     credential = get_credential_if_exists(eda_credential_id)