django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #13314 -- Make FileField validate the full filepath against max_length on ModelForms

Open anorthall opened this issue 5 months ago • 2 comments

Ticket: https://code.djangoproject.com/ticket/13314

This ticket has been laying about for over a decade and I decided I'd have a stab at closing it.

The problem

Django forms do not validate the full file path against the models.fields.FileField.max_length attribute, only against the file name of the file uploaded by the user. The file name of the file uploaded by the user and the actual file path provided to the database can be wildly different in length, due to the impact of themodels.fields.FileField.upload_to attribute, which can be dynamic.

If a file path is too long, it produces an error which will be presented as an Http500 to the user.

My approach

It makes sense that this validation can occur automatically in case of a ModelForm, where Django is able to easily link the forms.fields.FileField to the models.fields.FileField and access the generate_filename() method on it, to determine what the true length of the file path will be for any given file.

I have added two new attributes to forms.fields.FileField:

  • forms.fields.FileField._model_instance
  • forms.fields.FileField._model_field_name

The ModelForm constructor will automatically set these attributes when generating fields for a ModelForm, and as such proper validation will take place 'automatically'.

Concerns

  1. It is not possible to provide a helpful error message to the user of the form as it is not possible to determine how long their file name should be due to the dynamic nature of file path generation. For now I have settled with The filename is too long. Rename the file to something shorter., but perhaps something even more generic such as There was an issue uploading the file. should be used?
  2. Translation of any new error message will be required.

anorthall avatar Dec 05 '23 19:12 anorthall

@anorthall Thanks for this patch :+1: IMO, the only reasonable way to solve this is to introduce a new FileField kwarg (e.g. max_upload_to_length) where user could declare a length. Like max_digits and decimal_places work for DecimalField. There are of course backward compatibility concerns. I'm not sure it's worth fixing. Maybe we could just add a warning to docs that you should take the length of upload_to into account and limit forms.FileField.max_length :thinking:.

felixxm avatar Dec 11 '23 11:12 felixxm

@anorthall Thanks for this patch 👍 IMO, the only reasonable way to solve this is to introduce a new FileField kwarg (e.g. max_upload_to_length) where user could declare a length. Like max_digits and decimal_places work for DecimalField. There are of course backward compatibility concerns. I'm not sure it's worth fixing. Maybe we could just add a warning to docs that you should take the length of upload_to into account and limit forms.FileField.max_length 🤔.

I agree that it doesn't seem worth the added complexity of doing it 'automatically' (but you don't know until you try!) and that just highlighting the issue in the docs would likely be the best solution here.

I won't be coding for a few days now but I will come back to this and unless anyone has any other thoughts I will close this PR and submit a new one with a docs change.

anorthall avatar Dec 11 '23 11:12 anorthall