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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

msmagnanijr
Copy link
Contributor

@msmagnanijr msmagnanijr commented Jun 11, 2024

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 msmagnanijr requested a review from a team as a code owner June 11, 2024 13:34
@msmagnanijr
Copy link
Contributor Author

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

@bzwei
Copy link
Collaborator

bzwei commented Jun 11, 2024

@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?

@@ -57,6 +57,9 @@ class Meta:

def validate(self, data):
self.validate_shared_resource()
validators.check_if_team_name_exists(
Copy link
Contributor

Choose a reason for hiding this comment

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

@msmagnanijr When this resource gets managed in the Gateway does it follow the same constraints?

@bzwei
Copy link
Collaborator

bzwei commented Jun 21, 2024

@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
Copy link
Member

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants