fix(uploads): store local upload paths under the static mount - #293
Open
chattermate wants to merge 1 commit into
Open
fix(uploads): store local upload paths under the static mount#293chattermate wants to merge 1 commit into
chattermate wants to merge 1 commit into
Conversation
The app had two conventions for a stored local upload. store_upload wrote
`/api/v1/uploads/...`, matching the mount; agent photos and profile pictures
wrote a bare `/uploads/...`. The frontend resolves stored paths against the API
origin, so the bare ones came out as `https://host/uploads/...` and 404'd —
every agent and teammate avatar was broken on a self-hosted install using local
file storage. S3 deployments were unaffected, which is why it went unnoticed;
`/users/me/avatar` had already grown a one-off workaround for its own case.
Both writers now emit the prefix, and a migration brings existing rows over.
The three delete paths reconstructed the filesystem path with lstrip('/'),
which the prefix would have broken. They now share local_upload_path(), which
also replaces the marker-stripping copy-paste in load_upload/delete_upload and
tolerates the legacy shape for rows that predate the migration.
Signed-off-by: chattermate <admin@chattermate.chat>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #292, where this turned up.
Two conventions existed for a stored local upload.
store_uploadwrote/api/v1/uploads/..., matching the mount inmain.py; agent photos and profile pictures wrote a bare/uploads/.... The frontend resolves stored paths against the API origin, so the bare ones becamehttps://host/uploads/...and 404'd — every agent and teammate avatar broken on a self-hosted install with local file storage. S3 deployments store absolute URLs and were fine, which is why nobody hit it;/users/me/avatarhad already grown a one-off workaround for its own case.Both writers now emit the prefix, plus a migration for existing rows. It only touches values starting exactly
/uploads/, so S3 URLs, data URIs, NULLs and already-prefixed rows are left alone and re-running is a no-op — I verified that against Postgres, both directions.The three delete paths rebuilt the filesystem path with
lstrip('/'), which the new prefix would have broken (silently orphaning files). They now sharelocal_upload_path(), which also replaces the duplicated marker-stripping inload_upload/delete_uploadand tolerates the legacy shape.Scope note: you asked for
save_file, butusers.save_upload_filehad the identical defect and the same migration covers it, so fixing one and not the other seemed worse. Chat attachments also use a bare path — left alone, since they're served through a download endpoint that expects that shape, so nothing is broken there.Backend suite: 1440 pass, 6 pre-existing
TestSSHKeyLoadingfailures that also fail on main.