Python Django image upload API with 6 fixed slots

Question

I built an image upload API with Python Django. There are a fixed number of 6 slots, which can either be null or filled with something. There are two entities, Profile, which contains the six photos, and an overridden save method. The other entity involved is Upload, which contains the timestamp when it was uploaded at, and the file which is an ImageField.

The magic happens in views.py where the /users/profile/ request is processed. This request takes in optional image parameters photo1, photo2, photo3, ..., photo6. If one of these photo slots in the request are already filled, it calls the swap_photos() function which deletes the current photo from both the mediafiles/ directory and the Upload entity in the SQLite database, then creates a new photo in place of the deleted photo. The save process is quite complicated to avoid any ForeignKey constraint errors which I spent a considerable amount of time troubleshooting. Here is my code:

views.py:

def swap_photos(photo_obj, new_photo_val, user_profile, photo_id):
    """Deletes the photo in the old reference and adds the photo in the request"""
    Upload.delete(photo_obj)

    if photo_id == 1:
        user_profile.photo1 = Upload.objects.create(file=new_photo_val)

    elif photo_id == 2:
        user_profile.photo2 = Upload.objects.create(file=new_photo_val)

    elif photo_id == 3:
        user_profile.photo3 = Upload.objects.create(file=new_photo_val)

    elif photo_id == 4:
        user_profile.photo4 = Upload.objects.create(file=new_photo_val)

    elif photo_id == 5:
        user_profile.photo5 = Upload.objects.create(file=new_photo_val)

    elif photo_id == 6:
        user_profile.photo6 = Upload.objects.create(file=new_photo_val)


@api_view(['POST', 'PUT'])
def profile_create_or_update(request):
    """Creates or updates a profile"""
    user_profile = Profile.objects.filter(user=request.user).first()

    # Photo logic
    if 'photo1' in request.data:
        if user_profile.photo1 is not None:
            swap_photos(user_profile.photo1, request.data['photo1'], user_profile, 1)
        else:
            photo1 = Upload.objects.create(file=request.data['photo1'])
            user_profile.photo1 = photo1

    if 'photo2' in request.data:
        if user_profile.photo2 is not None:
            swap_photos(user_profile.photo2, request.data['photo2'], user_profile, 2)
        else:
            photo2 = Upload.objects.create(file=request.data['photo2'])
            user_profile.photo2 = photo2

    if 'photo3' in request.data:
        if user_profile.photo3 is not None:
            swap_photos(user_profile.photo3, request.data['photo3'], user_profile, 3)
        else:
            photo3 = Upload.objects.create(file=request.data['photo3'])
            user_profile.photo3 = photo3

    if 'photo4' in request.data:
        if user_profile.photo4 is not None:
            swap_photos(user_profile.photo4, request.data['photo4'], user_profile, 4)
        else:
            photo4 = Upload.objects.create(file=request.data['photo4'])
            user_profile.photo4 = photo4

    if 'photo5' in request.data:
        if user_profile.photo5 is not None:
            swap_photos(user_profile.photo5, request.data['photo5'], user_profile, 5)
        else:
            photo5 = Upload.objects.create(file=request.data['photo5'])
            user_profile.photo5 = photo5

    if 'photo6' in request.data:
        if user_profile.photo6 is not None:
            swap_photos(user_profile.photo6, request.data['photo6'], user_profile, 6)
        else:
            photo6 = Upload.objects.create(file=request.data['photo6'])
            user_profile.photo6 = photo6

    user_profile.save()
    return Response(status=status.HTTP_200_OK)

photos/models.py:

class Upload(models.Model):
    uploaded_at = models.DateTimeField(auto_now_add=True)
    file = models.ImageField()


    @property
    def is_valid(self):
        """
        A photo is valid if the datetime flag has value
        """
        return bool(self.uploaded_at)

    def delete(self, *args, **kwargs):
        super(Upload, self).delete(*args, **kwargs)
        os.remove(os.path.join(settings.MEDIA_ROOT, self.file.name))

    def save(self, *args, **kwargs):
        if not self.pk:
            super(Upload, self).save(*args, **kwargs)

    def __str__(self):
        return self.file.name

users/models.py:

import datetime

from django.contrib.auth.models import AbstractUser, BaseUserManager, PermissionsMixin
from django.utils.translation import gettext_lazy as _
from django.db import models
from django.forms import ValidationError

from photos.models import Upload

from .enums import get_choices, ReligionConstants, PoliticsConstants, \
    ChildrenStatusConstants, EthnicityConstants, SeekingConstants, PetsConstants, \
    AlcoholConstants, SmokingConstants, MarijuanaConstants, EducationLevelConstants


def age_from_birthday(birthday: datetime.date, /) -> int:
    return (datetime.date.today() - birthday).days // 365


class UserManager(BaseUserManager):
    use_in_migrations = True

    def _create_user(self, email, password, **extra_fields):
        """Create and save a User with the given email and password."""
        if not email:
            raise ValueError('The given email was not set')
        email = self.normalize_email(email)
        user = self.model(email=email, **extra_fields)
        user.set_password(password)
        user.save(using=self._db)

        profile = Profile.objects.create(user=user)
        profile.save()
        return user

    def create_user(self, email, password=None, **extra_fields):
        """Create and save a regular User with the given email and password."""
        extra_fields.setdefault('is_staff', False)
        extra_fields.setdefault('is_superuser', False)
        return self._create_user(email, password, **extra_fields)

    def create_superuser(self, email, password, **extra_fields):
        """Create and save a SuperUser with the given email and password."""
        extra_fields.setdefault('is_staff', True)
        extra_fields.setdefault('is_superuser', True)
        extra_fields.setdefault('location', {"town": "Jackson", "state": "New Jersey",
                                "country": "United States of America", "latitude": 40.09188, "longitude": -74.35875})

        if extra_fields.get('is_staff') is not True:
            raise ValueError('Superuser must have is_staff=True.')
        if extra_fields.get('is_superuser') is not True:
            raise ValueError('Superuser must have is_superuser=True.')

        return self._create_user(email, password, **extra_fields)


class User(AbstractUser, PermissionsMixin):
    """User characteristics that aren't expected to change often"""
    id = models.BigAutoField(primary_key=True)
    username = None
    email = models.EmailField(_('email address'), unique=True)
    first_name = models.CharField(max_length=16, default="")

    class Gender(models.TextChoices):
        MALE = "MALE", 'Male'
        FEMALE = "FEMALE", 'Female'

    gender = models.CharField(max_length=6, choices=Gender.choices)

    @property
    def age(self) -> int:
        return age_from_birthday(self.birthday)

    def age_validator(birthday):
        if not age_from_birthday(birthday) >= 18:
            raise ValidationError(_("User must be over 18"))

    birthday = models.DateField(validators=[age_validator])

    likes = models.ManyToManyField("self", blank=True)
    # LocationField()  # Town, State, Country, Latitude, Longitude
    location = models.JSONField(default=dict)
    # {"town": "Jackson", "state": "New Jersey", "country": "United States of America", "latitude": 40.09188, "longitude": -74.35875}

    date_joined = models.DateField(auto_now_add=True)

    force_reset_password = models.BooleanField(
        default=False, verbose_name="Force reset password on next login")

    USERNAME_FIELD = 'email'
    REQUIRED_FIELDS = ['first_name', 'birthday', 'gender']

    objects = UserManager()


class Profile(models.Model):
    """User characteristics that are expected to change often"""
    user = models.ForeignKey(User, on_delete=models.CASCADE)

    photo1 = models.ForeignKey(Upload, related_name='photo1', null=True, blank=True, on_delete=models.SET_NULL)
    photo2 = models.ForeignKey(Upload, related_name='photo2', null=True, blank=True, on_delete=models.SET_NULL)
    photo3 = models.ForeignKey(Upload, related_name='photo3', null=True, blank=True, on_delete=models.SET_NULL)
    photo4 = models.ForeignKey(Upload, related_name='photo4', null=True, blank=True, on_delete=models.SET_NULL)
    photo5 = models.ForeignKey(Upload, related_name='photo5', null=True, blank=True, on_delete=models.SET_NULL)
    photo6 = models.ForeignKey(Upload, related_name='photo6', null=True, blank=True, on_delete=models.SET_NULL)

    seeking_choice = models.CharField(choices=get_choices(SeekingConstants), default=SeekingConstants.BOTH_MEN_AND_WOMEN, null=True, blank=True, max_length=18)
    ethnicity = models.CharField(choices=get_choices(EthnicityConstants), null=True, blank=True, max_length=27)
    children_status = models.CharField(choices=get_choices(ChildrenStatusConstants), null=True, blank=True, max_length=25)
    religion = models.CharField(choices=get_choices(ReligionConstants), null=True, blank=True, max_length=20)
    politics = models.CharField(choices=get_choices(PoliticsConstants), null=True, blank=True, max_length=12)
    pets = models.CharField(choices=get_choices(PetsConstants), null=True, blank=True, max_length=7)
    alcohol = models.CharField(choices=get_choices(AlcoholConstants), null=True, blank=True, max_length=9)
    smoking = models.CharField(choices=get_choices(SmokingConstants), null=True, blank=True, max_length=9)
    marijuana = models.CharField(choices=get_choices(MarijuanaConstants), null=True, blank=True, max_length=9)
    education_level = models.CharField(choices=get_choices(EducationLevelConstants), null=True, blank=True, max_length=28)

    def save(self, *args, **kwargs):
        if self.photo1 is not None:
            self.photo1.save()

        if self.photo2 is not None:
            self.photo2.save()

        if self.photo3 is not None:
            self.photo3.save()

        if self.photo4 is not None:
            self.photo4.save()

        if self.photo5 is not None:
            self.photo5.save()

        if self.photo6 is not None:
            self.photo6.save()

        super(Profile, self).save(*args, **kwargs)

photos/models.py:

import os

from django.db import models
from django.conf import settings
from storages.backends.s3boto3 import S3Boto3Storage

# Create your models here.
class PublicMediaStorage(S3Boto3Storage):
    location = 'media'
    default_acl = 'public-read'
    file_overwrite = False


class Upload(models.Model):
    uploaded_at = models.DateTimeField(auto_now_add=True)
    file = models.ImageField()


    @property
    def is_valid(self):
        """
        A photo is valid if the datetime flag has value
        """
        return bool(self.uploaded_at)

    def delete(self, *args, **kwargs):
        if os.environ.get('DJANGO_SETTINGS_MODULE') == 'config.settings.local':
            super(Upload, self).delete(*args, **kwargs)
            os.remove(os.path.join(settings.MEDIA_ROOT, self.file.name))
        elif os.environ.get('DJANGO_SETTINGS_MODULE') == 'config.settings.dev':
            super(Upload, self).delete(*args, **kwargs)
            # os.remove(os.path.join(settings.MEDIA_ROOT, self.file.name))

    def save(self, *args, **kwargs):
        if not self.pk:
            super(Upload, self).save(*args, **kwargs)

    def __str__(self):
        return self.file.name

What are some more efficient ways to write this code out for both increased readability, maintainability, performance, and security if necessary?

Answer 1

more efficient ways to write this code out for both increased readability, maintainability ?

My goodness! Bet your keyboard looks like [Z][X][C][ ][B], with the V markings rubbed off by repeated paste paste paste.

When you see repetition, ask how you can parameterize or refactor.

Guido gave us lists and dicts -- use them!

---

setattr

def swap_photos( ...
    if photo_id == 1:
        user_profile.photo1 = Upload.objects.create(file=new_photo_val)
    ...
    elif photo_id == 6:
        user_profile.photo6 = Upload.objects.create(file=new_photo_val)

All the ifs boil down to if photo_id in range(1, 7):, that's easy.
(Or you might prefer if 1 <= photo_id < 7:.)

I will grant you that if we retain this API exactly it might seem a little hard to parameterize. The magic builtin that we use less often than a dict is called setattr. Here, you can use

    setattr(user_profile, f"photo{photo_id}", Upload.objects.create(file=new_photo_val))

---

items()

In profile_create_or_update it appears you want to loop over some dictionary items and use the getattr builtin:

import re

photo_re = re.compile(r"^photo\d+$")
...
def profile_create_or_update( ...

    for k, v in request.data.items():
        if photo_re.search(k):  # key looks like "photo" with numeric suffix
            if (photo := getattr(user_profile, k, None)) is not None:
                swap_photos(photo, v, user_profile, 1)
            else:
                photo = Upload.objects.create(file=v)
                setattr(user_profile, k, photo)

I confess that all those {get,set}attr calls are a little inconvenient. Maybe we could convince user_profile to offer a nicer Public API? Or we might prefer to maintain a dict which we then copy into user_profile at the last moment.

---

positional_or_keyword

def age_from_birthday(birthday: datetime.date, /) -> int:
    return (datetime.date.today() - birthday).days // 365

Ummm, you kind of lost me there with the / slash. There's a PEP-570 thing going on here, where you're prohibiting caller from mentioning ... , some_name=42) arguments. Except that, there's no more parameters after the slash. I don't get it. The slash only seems useful if it's followed by one or more params. Recommend you delete it, or (minimally) that you describe the motivation in a comment.

Also, nit, some years have 365 days. For people who have managed to make it through at least one year of primary school, we tend to see them racking up the occasional February 29th. There's lots of details. You did great by accessing a timedelta's .days property. Better to examine a relativedelta's .years property. Then we report accurately, even on days near the person's birthday.

---

schema design

Imagine one day the marketing director walks in and announces that "tomorrow we have to launch a seventh photo feature!" Or eight photos or some other nightmare integer. In how many places will we need to adjust the code? Yeah. Quite a few. I suspect the design took a turn for the worse when we started using six columns in a certain table.

Consider normalizing the DB design so there's a table entirely devoted to photos, with FK pointing back to profile.

Now the bits of {getattr, setattr} ugliness above disappear, and adjusting max photos could involve editing just a single line of code.

---

This is pretty OK code. I didn't see anything terrible in it. It just suffers from too much repetitive pasted source text.

This code appears to achieve its design goals.

I would be willing to delegate or accept maintenance tasks on it.