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

Wrong design of Base64 file fields #111

Open
beltiras opened this issue Oct 31, 2019 · 6 comments
Open

Wrong design of Base64 file fields #111

beltiras opened this issue Oct 31, 2019 · 6 comments

Comments

@beltiras
Copy link

I understand how you arrive at this design because it serves the usecase of checking if an image is an image file and that image files have few extensions. It makes the generic component very hard to use if you simply need something that serializes/deserializes base64 for storage or retrieval. It's reminiscent of Interfaces in Java.

At most you should take these functions as parameters so that one can provide a lambda on field construction.

Most of the time I wouldn't have spent the time making an issue, just ripped the code I needed and gone about my day. This seems to be a good library thou and I'm willing to put in effort to make it better.

I don't know how to best go about it. A discussion on a goal seemed to be the right place to start.

@beltiras
Copy link
Author

At the very least one should be able to provide '*' as a parameter to bypass the check. That would be easy enough to implement.

@beltiras
Copy link
Author

I'm reading https://github.com/Hipo/drf-extra-fields/blob/master/drf_extra_fields/fields.py#L52 and unless I am reading it wrong this code will never do anything at all. It makes a uuid4 to use as a filename and then checks if the extension (!!) of the uuid4 string is in ALLOWED_TYPES. This results in a contradiction.

@alicertel
Copy link
Contributor

Hey @arnists thanks for your contributions.

At the very least one should be able to provide '*' as a parameter to bypass the check. That would be easy enough to implement.

Yes, you are right. We have an open PR #72 but it's not completed, documentation and tests are missing. Would you like to help on that?

file_name is not being used while determining the extension of the file, decoded_file is being used. Here you can check how what works.

@alicertel
Copy link
Contributor

Hi @arnists Do you have any suggestion to improve design of the Base64 fields? I am happy to hear your suggestions?

@beltiras
Copy link
Author

I do. I didn't get a lot of sleep tonight so I couldn't make a relevant contribution today. I ended up reworking the field in my project and using it changed a bit. I'll see if I can make a PR tomorrow with my code. I'll also see if I can make a more sweeping architectural change.

@vladdoster
Copy link

youd want something like this @arnists

fields.py

import base64
import uuid

from django.core.files.base import ContentFile
from rest_framework import serializers
from rest_framework.fields import SkipField


class Base64FieldMixin(object):

    def _decode(self, data):
        if isinstance(data, str) and data.startswith('data:'):
            # base64 encoded file - decode
            format, datastr = data.split(';base64,')    # format ~= data:image/X,
            ext = format.split('/')[-1]    # guess file extension
            if ext[:3] == 'svg':
                ext = 'svg'

            data = ContentFile(
                base64.b64decode(datastr),
                name='{}.{}'.format(uuid.uuid4(), ext)
            )

        elif isinstance(data, str) and data.startswith('http'):
            raise SkipField()

        return data

    def to_internal_value(self, data):
        data = self._decode(data)
        return super(Base64FieldMixin, self).to_internal_value(data)


class Base64ImageField(Base64FieldMixin, serializers.ImageField):
    pass


class Base64FileField(Base64FieldMixin, serializers.FileField):
    pass

serializers.py

from django.db import models

from rest_framework.serializers import (ModelSerializer as DRFModelSerializer,
                                        HyperlinkedModelSerializer as DRFHyperlinkedModelSerializer)

from .fields import Base64FileField, Base64ImageField


class Base64ModelSerializerMixin(object):

    def __init__(self, *args, **kwargs):
        self.serializer_field_mapping.update({
            models.FileField: Base64FileField,
            models.ImageField: Base64ImageField,
        })
        super(Base64ModelSerializerMixin, self).__init__(*args, **kwargs)


class ModelSerializer(Base64ModelSerializerMixin, DRFModelSerializer):
    pass


class HyperlinkedModelSerializer(Base64ModelSerializerMixin, DRFHyperlinkedModelSerializer):
    pass

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

No branches or pull requests

3 participants