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

Support for Uploads when using Django Form #30

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

Conversation

GasimGasimzada
Copy link

@GasimGasimzada GasimGasimzada commented Apr 9, 2019

This is a Work In Progress PR w/ preliminary source code to show what I am trying to achieve and would like a feedback before I move forward with this idea.

Problem

Django Forms work in a different way when it comes to uploads. The uploads must be sent in the second argument of the Form constructor. The FILES object is typically a dictionary where the keys match the field names. If you send the file object key in the first argument, the file is ignored from the form:

some_form = SomeModelForm(request.POST, request.FILES)

However, the Apollo Upload specification handles uploads in a different way. Firstly, the request.FILES object becomes an ordered dictionary of values such as { "0": MemoryUploadedFilehandler(...) }. Secondly, as far as I understood from specs and the source code, the "map" field helps the server to merge the uploaded files into a single field (as array or just one object). This works great when dealing files right inside of GraphQL/Graphene mutations. However, the way Django Forms and this library handles files is not aligned.

Solution

I have decided to play with this idea (when I started with this idea, I was not aware of graphene-file-upload library but this example code will work regardless) and came up with a solution. I have written the source code that works with Django's ModelForm and I tried to make sure that the way the mutation classes are implemented are similar in architecture to Graphene Django's mutations. I have copied some things from Graphene Django docs to scaffold my idea because it was not possible to properly extend Graphene Django's model to only add the necessary logic; however, I want to refactor it to extend instead of replace these classes.

In the source code below, I am doing two things:

First and foremost, I am registering Django Form's FileField (ImageField is derived from FileField; so, mapping one field is enough) class to accept Upload type. This is because, by default FileField accepts only Strings and we will get an error (__init__.py line 11-13). I put this register in __init__.py; so that, the types are mapped as soon as you import the view custom GraphQL view.

Then, I created a custom class DjangoUploadModelFormMutation that works exactly like DjangoModelFormMutation but tries to extract fields that are instances of FileField and store it in an array. Before Django Form is created for validation, the stored file field names are used to extract files from the input object. The extracted values are then sent as a second argument to Django Form (mutation.py line 50-53). I had to rewrite that part because Graphene Django uses keyword arguments to pass form data but I have failed to find a way to pass files as keyword arguments as well. So, I had to override get_form method instead of overriding get_form_kwargs method.

Usage

When doing it this way, the usage becomes much easier because the developer does not need to write anything other than change the class name:


class PersonForm(ModelForm):
   ...
   photo = ImageField(upload_to='images/')

class CreatePersonMutation(DjangoUploadModelFormMutation): # Just replace the class name 
    class Meta:
        form_class = PersonForm
        exclude_fields = ('id',)

Of course this usage would be different for non Model Form but I think it is possible to make it as seamless as possible for existing codebases; so that, devs can work with validated form data instead of mutation inputs.

Things to do

  • Write models for Django Form (not ModelForm)
  • Refactor ModelForm to properly extend from Graphene Django mutations
  • Write tests
  • Write docs showing usage with Django Forms

If you would like this integration to be part of the library, I am willing to help with this.

- Register Django Form FileField as Upload
@wmai
Copy link

wmai commented Jul 7, 2019

Any updates on this? I would be so happy to have it integrated with my Django forms.

@GasimGasimzada
Copy link
Author

@wmai I am currently using a modified version of the codebase that is in the forked repo but got no response on this. If there is a necessity, I can keep working on it and we can merge it to original repo.

@davidroeca
Copy link
Collaborator

@GasimGasimzada I'm not a django user, which is why I haven't replied. If you're up to the task of maintaining this aspect of the library, maybe we can go through the process of releasing a few "unstable" (beta/release candidate) versions until we feel the interface has stabilized. This may also pose challenges when graphene upgrades to the latest graphql-core-next versions so we should keep an eye on those changes and make sure the base of this library still works even if these more complicated django-form implementations break. We'd need help from @lmcgartland to either give one of us pypi access or for him to continue managing the releases.

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.

4 participants