ansible / eda-server

Event Driven Ansible for AAP
Apache License 2.0
66 stars 42 forks source link

Creating a new Team with the same name should not propagate IntegrityError #934

Open msmagnanijr opened 3 months ago

msmagnanijr commented 3 months ago

When attempting to create a team with the same name as an existing team within the same organization, the API currently returns a HTTP 500 error. While this correctly identifies this, the error handling can be improved to provide a more user-friendly response.

msmagnanijr commented 3 months ago

Screenshot from 2024-06-11 10-47-23

bzwei commented 3 months ago

@msmagnanijr I feel we should not add this specific uniqueness constraint validation in Team serialization, otherwise we would need to do the same for every serialization that has a uniqueness constraint field (or joint fields). Let's find out why only this one yields a 500 error. How do others handle django.db.utils.IntegrityError?

bzwei commented 3 months ago

@msmagnanijr Because in the serializer we introduce organization_id to replace the organization in the model, DRF does not auto apply UniqueTogetherValidator based on the model's uniqueness constraint on organization and name jointly. If you switch to use organization in the serializer, you will get a 400 error with a nicely formatted string {"non_field_errors":["The fields organization, name must make a unique set."]}. This is exactly what we see when we use gateway api for the same test.

Since we will continue to use organization_id the same way as we do to all other serializers for foreign keys, I would follow the same general handling as gateway does, adding the following under this line

# from rest_framework.exceptions import ParseError
# from django.db import IntegrityError
if response is None:
    if isinstance(exc, IntegrityError) or isinstance(exc, FieldError):
        exc = ParseError(exc.args[0])
        response = exception_handler(exc, context)

The response will be a 400 error with data {'detail': ErrorDetail(string='duplicate key value violates unique constraint "core_team_organization_id_name_e81b4a73_uniq"\nDETAIL: Key (organization_id, name)=(1, test-team) already exists.\n', code='parse_error')}. Unfortunately the message is very technical not user friendly.

Dostonbek1 commented 3 months ago

@bzwei @msmagnanijr We could also add the following in TeamCreateSerializer under Meta:

      validators = [
          UniqueTogetherValidator(
              queryset=models.Team.objects.all(),
              fields=["organization_id", "name"]  # --> changing org field to 'organization_id' here 
          )
      ]

which would return the 400 error message @bzwei mentioned above:

{
    "non_field_errors": [
        "The fields organization_id, name must make a unique set."
    ]
}
msmagnanijr commented 3 months ago

@Dostonbek1 @bzwei thanks! I'll work on it now and ask for a new review.