Direct Attachment Uploads#48
Conversation
gracewhitney
left a comment
gracewhitney
left a comment
There was a problem hiding this comment.
Bunch of comments, sorry 😬 many of them are not specific to file uploads, but since this is a reference codebase want to make sure we're keeping everything clean
| <div class="card-body"> | ||
| <h3 class="card-title">All Attachments</h3> | ||
| <file-upload-dashboard | ||
| queryset_json="{{ attachments|escape }}" |
There was a problem hiding this comment.
🎨 escape isn't doing anything here - it's already autoescaped. I don't think it's a good idea to inline the json as a param like this, though - won't the quotes in the json blob interfere with html parsing? It would be better to use json_script and provide an id to the element containing the JSON. Or, just populate async from an internal API path.
There was a problem hiding this comment.
I have experimented a little with using json_script in various places, and I agree it is better from a stability and security perspective, but I can't seem to find a good pattern to work with both this use case (rendering a list of attachments on the dashboard) and in the custom django form field case simultaneously. In the latter case, you really want to collocate the existing attachments context (in other cases, for instance in a Model multi-select widget, you'd want the serialized choices queryset), and if the form is within a vue app, vue will strip the <script> tags, and I haven't really found a good way to circumvent that. To that end, passing the context directly in props seems to be the cleanest and most django-like way to go.
I have cleaned up the passing of variables here though, I now directly consume the queryset json in the files vue model rather than using an intermediate variable, and i've removed the escape template tag (I agree I don't think that does anything here).
❓ Do you have any other ideas for a good pattern that achieves both of these goals?
| created_by = models.ForeignKey(User, related_name="sample_objects", on_delete=models.PROTECT) | ||
|
|
||
| # START_FEATURE direct_upload | ||
| attachments = models.ManyToManyField("Attachment", related_name="sample_objects") |
There was a problem hiding this comment.
❓ Why is this a many to many instead of a FK? I suppose there could be a case for M2M but seems less likely, not that it particularly matters for the example
There was a problem hiding this comment.
This code originates from DPU use-cases, and on that project MTM is very much preferred, not only in linking attachments but also in linking almost every other model. I would potentially be interested in implementing both methods (FK + MTM) since this is a reference/example implementation, if you think that would be useful
| model = Attachment | ||
| exclude = [] | ||
|
|
||
| def to_representation(self, instance): |
There was a problem hiding this comment.
❓ Do you prefer overriding to_representation instead of using SerializerMethodField?
There was a problem hiding this comment.
I think it depends on the situation, but in this case I think it would be much more verbose and provide no real benefits (its not like serializers are type-hinted anyways
Edit: Actually if it is explicit, then drf-spectacular would be able to generate a better openAPI spec so that would be a decent benefit
| # END_FEATURE vue | ||
| # ---------------------------------- Python ---------------------------------- # | ||
|
|
||
| FROM python:3.12.13-slim-trixie |
| " \ | ||
| && deps=" \ | ||
| postgresql-client \ | ||
| git \ |
There was a problem hiding this comment.
Do we need git on the server? If you need it in a particular instance you can always install it, but doesn't seem necessary in general
| # Copy application files and .env.example | ||
| COPY . /app/ | ||
| COPY ./config/.env.example /app/config/.env | ||
| COPY .bashrc /root/ |
There was a problem hiding this comment.
❓ I don't see a .bashrc file in this codebase - won't this fail if not present?
| CMD ["gunicorn", "--bind", ":8000", "--workers", "3", "config.wsgi:application"] | ||
| # END_FEATURE docker | ||
| EXPOSE 8080 | ||
| CMD ["gunicorn", "--bind", ":8080", "--workers", "15", "config.wsgi:application"] |
There was a problem hiding this comment.
15 workers? We're using 2 workers w/ 5 threads each.
| # END_FEATURE django_storages | ||
|
|
||
| # START_FEATURE direct_upload | ||
| boto3 |
There was a problem hiding this comment.
Not needed if you replace the boto3 calls with default_storage calls. Also need to pip-compile
2 participants
No description provided.