Form support lacks file upload

[nerdoc]

While FormComponent works reasonably well, It completely lacks file upload support. The problem is separated in a few parts:

• HTML seems to need a form tag with an enctype="multipart/form-data" attribute to allow file uploads
• There must be a POST request. Subsequent GET requests loose the file again - it must be kept track of in between (it's deleted/GCed if the user does not finish the form)
• I think the relevant code parts are in tetra/js/tetra.core.js - this is the main part where all the Js magic of Tetra happens - note: if you change something there locally, call tetra/build_js.sh to update the static min files and debug maps automatically.
• I think the callServerMethod() and getStateWithChildren(...) methods closely resemble the code @waqasidrees07 wrote to upload the file in his PR #70. But it has to be done in tetra.core.js. In the test_tetra_forms repository, there should ideally only be changed some minimal things like an added <form enctype="multipart/form-data"></form> tag.

[nerdoc]

Maybe this is usable: https://dev.to/code_rabbi/programmatically-setting-file-inputs-in-javascript-2p7i

[nerdoc]

@gsxdsm Basically, in the form_upload branch, Tetra creates a x-model attribute automatically when creating the form component, for each form field. I first really thought that it is an Alpine bug and filed an issue there, just to learn that this is normal HTML behaviour: file input fields can't have x-model attrs.

As far as I found out, like stated above, I think that it should go this way.

• when a user adds a file to an file input field, Tetra should capture that changed event and upload the file to a temporary place (after checking the FormField constraints like size, ending etc.
• Upload must be done by tetra.core.js using Js FormData() - and not the normal form upload process - this is where @waqasidrees07 did wrong). a <form> tag MUST NOT be necessary, as Tetra doesn't enforce it. In fact, <form> tags create more problems, as they cause a full site reload, if not prevented actively. AFAIK the callServerMethod() and getStateWithChildren(...) methods are the best places for that - but I for myself don't have the Js skills to work that out well.
• when the FormComponent is validated wrong and HTML gets updated, the filename should remain as "value" in the file input field, so that it is visible the field has a value.
• When the component is submit()ted, and validated ok, the file could be moved to it's permanent place.
• There should be some garbage collection of files in the temporary directory, to be deleted. CAVE: race slow conditions, when the "GC" deletes "old" temp files, and someone is in the upload process and just uploaded a file into the temp. folder. I think there could be a (Tetra internal) TempFile model helpful to mark the upload time into the tmp folder, and GC only deletes files that exceed a certain age, e.g. 24h.

[nerdoc]

i pushed what I have in my local form_support branch.

[gsxdsm]

Got it thanks for the detailed breakdown. Starting to dig in there!

[nerdoc]

@gsxdsm Oh, and in case you didn't see it: I started a small example project to test the file upload: https://github.com/tetra-framework/test_tetra_forms Maybe you can use that.

[gsxdsm]

Excellent thank you!

[nerdoc]

@gsxdsm Did you make any progress? I have time tomorrow to help out here/code - if you tell me what you are doing ATM, I could drop in and help a bit?

[gsxdsm]

Not yet, have been a bit swamped - no notable progress. I have been looking at Laravel Livewire to see how they approach this, but nothing concrete.

[gsxdsm]

Still looking into this, not making much progress but will keep you updated this weekend

[gsxdsm]

Have been thinking about the cleanup of the temporary files - don't want to take another dependency here, but I'm thinking we need some sort of job/task. Did you have thoughts on the mechanism for the cleanup?

[gsxdsm]

Maybe kicking off something asynchronously from runserver?

[gsxdsm]

Okay, a quick test of creating an async job spawned from runserver to run at a period interval to do the cleanup seems to work. With this in place, I'll work on the rest of the upload process.

[nerdoc]

You mean a regular job? Because runserver isn't called in production (when using gunicorn, Daphne etc). I think I wouldn't bother too much ATM when this job is executed, but just make some cleanup procedure that actually does the job. It could be up to the Dev when this cleanup happens, starting from a poor-man's-cron to a celery task. IMHO it's enough when the procedure is available...

[gsxdsm]

Sounds good, will take that approach and focus on getting the rest working.

[nerdoc]

please tell me if I can assist. the form_support branch has already some code in it towards that aim, but not very much. If you add a PR, please do against that branch.

[gsxdsm]

Yes will do - I am working off of the form_support branch and the form test project, thanks! It is fairly complex, will keep you posted if I get stuck further.

[nerdoc]

As @waqasidrees07 got stuck in a very early stage, please feel free to contact me at any stage, so you don't make the same errors again as I did ;-)

[gsxdsm]

Quick update:

I have the change event being fired in Javascript and testing the file upload using the FormData (rather than a form tag).

Took a while to get my head around the overall flow, but thanks for the pointers - I see a path now to implementing. At least for single files! (Multi will need to come later)

[gsxdsm]

Getting closer! I have the javascript posting the file to a server method for the temporary upload on change! Debugging a bit, but making progress.

[gsxdsm]

Here's where I am:

• [x] when a user adds a file to an file input field, Tetra should capture that changed event and upload the file to a temporary place

• [ ] after checking the FormField constraints like size, ending etc.

• [x] Upload must be done by tetra.core.js using Js FormData()

• [x] when the FormComponent is validated wrong and HTML gets updated, the filename should remain as "value" in the file input field, so that it is visible the field has a value.

• [x] When the component is submit()ted, and validated ok, the file could be moved to it's permanent place.

• [ ] There should be some garbage collection of files in the temporary directory, to be deleted. CAVE: race slow conditions, when the "GC" deletes "old" temp files, and someone is in the upload process and just uploaded a file into the temp. folder. I think there could be a (Tetra internal) TempFile model helpful to mark the upload time into the tmp folder, and GC only deletes files that exceed a certain age, e.g. 24h

---

The hardest part was getting the interplay between the server method and javascript working, but that's all done. The rest should be pretty straightforward.

One question - do we want:

1. A task that can be run via manage.py to cleanup stale temp files?
2. Stale temp files to be cleaned up whenever a new temp file upload happens (this is what livewire does)
3. Both 1&2 -> benefit, the user will never have temp files accumulate if they forget to run the cleanup job or fail to setup a scheduler

I'm inclined to do #3, but want to get your POV.

Also, I'm thinking the TTL for the temp files should probably be a default of maybe 15 minutes? But should be overridable in settings?

Finally - I was going to make this work for multiple files, but I now realize django doesn't really support multiple files in a single FormField, right?

[nerdoc]

That's great news, really. About your question:

I think that a GC procedure should be available definitely as management command, on the long term it could be a subcommand of "tetra", like manage.py tetra init, manage.py tetra startcomponent, and here: manage.py tetra clean. This could trigger the same procedure as the poor-mans-GC with e.g. during file upload. Attention, here the race condition could occur.

I think I would really leave it up to the user. There may be use cases where the upload-auto-GC makes sense, in some (more critical) environments this would be a no-go...

So it would be absolutely ok if you just implement the GC procedure (e.g. in tetra.utils), I (or you?) can make a makagement command later too.

[gsxdsm]

I'm thinking through the step of moving the file from temporary storage to its permanent location and I'm guessing we'd want to use the FileField.upload_to property, right? (https://docs.djangoproject.com/en/5.1/ref/models/fields/#django.db.models.FileField.upload_to). If this is the case, I'd assume we'd also use the storage method on the File (or Image) field as well, right? We'd probably want to use the storage method on the file (or default) for the temporary file storage too.

Or did you have something more customizable/user controllable in mind?

[gsxdsm]

Okay, have the file moving and deletion of temp file working now. Tomorrow I will cleanup and submit something in the branch to take a look - we can iterate on that while I work on validation and the garbage collection.

[gsxdsm]

Actually I just put together a quick commit now to show what's done so far so you can provide feedback @nerdoc - would love feedback/suggestions. I need to do a lot more testing and refinement, but the basic approach is here

[nerdoc]

That's great, exactly how I thought it should be. I just hat a few minutes to look over the code, did not test it - but AFAICT this is definitely the way to go. Thanks, really. That was a few months of agony that comes to an end now ;-)

[nerdoc]

2 Things came into my mind while reading the code:

• The callServerMethod() and callServerMethodWithFile() are mostly dup code and may be merged - but maybe you had a good reason to use a separate method "with files".
• could the _uploadFile() Js method be a security risk? by uploading files (to at least a temp dir?)

Just to be kept in mind.

[gsxdsm]

Yes - I can merge them, I started with them merged and debated splitting it up or not, but will merge back.

Yes, _uploadFile is definitely something that needs a bit of scrutiny. The csrf check helps a little, but I am going to take another look at livewire's approach to see if there are additional checks/holes there. One quick thing I was thinking was to have the method become a no-op if the component doesn't have a FileField...

[nerdoc]

Yes, that would be some additional security. Maybe you could add just a TODO in the callServerMethodWithFile() so we don't forget the security checks at the backend side.

[gsxdsm]

Updated with a todo for security checks and restricted the upload endpoint to only components with a file field