TUS IMPLEMENTATION #36
Conversation
|
Bitte überprüft auch wie es mit Lizenz sein soll. Ich bin mir da unsicher. |
|
Is there any news? |
|
Could you provid an apk for tester who are not able to compile it? |
|
|
fix(cache): add LruCache and null-safe disk cache init for thumbnails
|
|
opencloudApp/src/main/java/eu/opencloud/android/datamodel/ThumbnailsCacheManager.java:72 ergänzt einen threadsicheren In-Memory-LruCache samt Lock, prüft Schlüssel/Bitmap vor dem Schreiben, generiert robuste Cache-Keys für OCFile und fällt beim Disk-Cache auf das interne Verzeichnis zurück, falls extern nicht verfügbar. |
|
fix(auth): OIDC-Login ohne Dynamic Registration ermöglichen
|
|
feat(upload): honor TUS capabilities and refine fallback logic
Why: improve interoperability with varying servers/proxies, respect |
Danke! Läuft wie geschmiert jetzt ;-) |
Probier die neueren Versionen im release, die cachen nun die Thumbnails etc um die Last zu verbessern, uploads sind zuverlässiger |
|
@zerox80 Sorry for delay. Before we can do a deeper review, here are some pre comments from me:
Please ping when you updated the PR :) |
|
Wait, why from August? zerox80 commented last week Read filesTusSupport from capabilities and pass into TUS flow This solves lot of issues, for example using 5G Network etc. Ill keep this one aswell. |
|
PR updated! :) Squashed August commits, removed OIDC & permission/PNG changes, clean commit history with TUS + Thumbnails separated. |
|
And i apologise for mixing up with PR's, it won't happen in future (the stuff with Thumbnail caching in a PR with TUS is kinda bullshit i know) :) let me know about updates! Thanks. |
zerox80
force-pushed
the
main
branch
2 times, most recently
from
47e002e to
e99a96e
Compare
|
Thanks, will have a look sometime today :) |
|
Fixed gradle build :) Couldnt build it otherwise. |
guruz
left a comment
guruz
left a comment
There was a problem hiding this comment.
Hi, my comments are added to the individual lines.
I have only looked at the first commit dd872cd so far and not at your other stuff .
Regarding what you mean about Progress display, I think the function getRunningUploadsWorkInfosLiveData might be relevant..
| } | ||
|
|
||
| if (!usedTus) { | ||
| if (isChunkingAllowed && fileSize > CHUNK_SIZE) { |
There was a problem hiding this comment.
@TheOneRing says that actually the old chunking is being phased out. So basicaly, files are uploaded either with a single PUT or with TUS. So you can kick out this handling.
There was a problem hiding this comment.
Thanks for pointing that out. With the current OpenCloud, we no longer need the old WebDAV chunking as a fallback. I'll adjust the PR so that we only try TUS for large uploads and fall back to a simple PUT otherwise.
| removeCacheFile() | ||
| } | ||
|
|
||
| private fun uploadTusFile(client: OpenCloudClient) { |
There was a problem hiding this comment.
You have quite similar code in this file here and in UploadFileFromSystemWorker. Did you check if this is OK? It might be OK or it might not.
There was a problem hiding this comment.
The point with the double TUS code is valid. I extract the common logic into a small helper so that
UploadFileFromContentUriWorker
and
UploadFileFromFileSystemWorker
use the same process.
...src/main/java/eu/opencloud/android/lib/resources/files/tus/CreateTusUploadRemoteOperation.kt
Outdated
Show resolved
Hide resolved
| val idx = initial.indexOf("/uploads/") | ||
| if (idx > 0) initial.substring(0, idx + "/uploads/".length) else withSlash | ||
| } | ||
| val altInitial = initial.replace("/remote.php/dav/", "/dav/") |
There was a problem hiding this comment.
Updated this as well. The OPTIONS request now just hits the uploads collection (base + optional trailing slash) and drops the manual /remote.php replacements. That keeps the detection logic in line with what the backend serves and avoids the brittle string juggling.
| } | ||
|
|
||
| // Fallback probe on this endpoint: try POST with minimal test | ||
| Timber.d("TUS headers inconclusive at %s, probing with POST test", endpoint) |
There was a problem hiding this comment.
I don't think a TUS probe is necessary. OpenCloud always supports TUS(?).
There was a problem hiding this comment.
Makes sense. Since the capabilities endpoint already advertises TUS for OpenCloud I removed the POST probe entirely and only evaluate the headers. If the OPTIONS call fails we now just return “unsupported” without firing any extra requests.
| // TUS Upload-Metadata (decoded for debugging) | ||
| val filename = remotePath.substringAfterLast('/') | ||
| val mtime = (file.lastModified() / 1000).toString() | ||
| val checksum = "sha1 " + computeSha1(file) |
There was a problem hiding this comment.
Why is there a sha1 computed here?
Is the checksum (sha256) not coming from the caller of this function?
Maybe I'm misunderstanding something
There was a problem hiding this comment.
Yep, that was redundant. I removed the internal SHA‑1 and leave the checksum field to whatever the caller passes in the metadata (we still ensure filename + mtime defaults). The helper now simply forwards those values to the TUS POST.
| "notNull": false | ||
| }, | ||
| { | ||
| "fieldPath": "tusUploadOffset", |
There was a problem hiding this comment.
Haven't look at this in detail, but does it make sense to have the Offset stored in the LOCAL db? Doesn't the server need to tell us what he received last?
Or is this related to multithreaded workers somehow and I dont understand it yet
There was a problem hiding this comment.
...because the server has the definitie state of what it received before a disconenct/timeout/network issue. The clients view might be incorrect and further ahead or whatever
There was a problem hiding this comment.
Good point. I dropped the local tusUploadOffset field and always resume from the server state. The helper now calls GetTusUploadOffset (with our standard executeRemoteOperation) whenever we recover or resume, so we stay in sync with the backend
|
|
||
| // Try to recover by re-checking current offset from server and continue | ||
| val recover = try { | ||
| val off = GetTusUploadOffsetRemoteOperation(tusUrl!!).execute(client) |
There was a problem hiding this comment.
Do all those .execute calls not need to be wrapped into executeRemoteOperation(...) to get better exceptions?
Analog to the existing code..
There was a problem hiding this comment.
Yep, I routed the head/creation/recover calls through executeRemoteOperation inside TusUploadHelper, so we still get the mapped exceptions while keeping the loop logic intact.
| init { | ||
| require(chunkSize > 0) { "Chunk size must be greater than zero" } | ||
| } | ||
|
|
There was a problem hiding this comment.
What are the changes in this file about?
|
The old WebDAV chunking logic has been removed – we now only try TUS (if the file size is sufficient) and otherwise fall back to a simple PUT. |
|
I think its problematic that removeLocal is set to true boolean, because the original file gets deleted after upload. I set it to false. |
|
I have addressed the outstanding issues in the latest commits. Could you please take a look at the current overall status? The older commits now contain outdated changes. |
ups, sorry
|
|
@guruz Can u test again? |
|
@guruz detekt should pass now. |
- Add TUS upload operations (create, chunk, offset check, support detection) - Implement chunked upload with resume capability - Add file list adapter, thumbnail requester, and file details improvements - Fix unit test failures on Windows (path separator compatibility) - Upgrade MockK to 1.13.13 for Java 21 support - Add Gradle Version Catalog for dependency management - Fix Detekt issues and build warnings
|
settings local is for my IDE i committed it accidently. |
|
Ok, its all green :) |
|
Ups! I reverted. pushed to wrong branch :) |
FloatingActionButton inflates our action icons without a themed Context. With targetSdk 34/35 the ?attr/colorControlNormal tint in the vectors cannot resolve and the app crashes during Splash/FileDisplayActivity startup (see unresolved theme attribute warnings). Remove the theme-dependent tint from ic_action_create_dir.xml, ic_action_create_file.xml, ic_action_open_shortcut.xml and let FAB apply its own tint. No code changes; verified with assembleDebug.
|
@guruz I found the crash issue. Its solved now - for newer SDKs. So future crashes for other PRs won't happen. Please review and merge |
|
I can confirm it works :) (hooray!) One thing I notice in the log output is that it runs GetTusUploadOffsetRemoteOperation directly after CreateTusUploadRemoteOperation? Is the iOS and the desktop doing this the same way? |
|
Hi, u can merge this, and we can discuss later about this, is this fine? |
|
I can make a future PR if its really needed, but the risk ruining a almost 4k lines of Codebase change is too much, cuz the Code works fine now... |
Thank you for that. From my understanding it's not even needed - all of the code is automatically GPL because of copyleft. OpenCloud doesn't do dual licensing (gpl + commercial or something similar), so from my understanding everything is already good, even without your comment. |
Okay - I am confused because i see a lot of "Owncloud" and "Author" names :) But its ur choice, feel free |
|
@guruz I checked the code ( So the Android implementation can indeed be optimized here, but as you suggested, let's do that in a follow-up PR to keep this one focused. |
we're just not allowed to remove them from the fork. we don't need them for ourselves afaik. |
Yes 100% please keep this PR as it is. |
Would be good if u could do it very soon, because 2 other PRs can be rebased to the main branch then so i can detekt and test the codes much better so it works next time out of box for u ^^ |
|
For some other reason, i just started a current apk on an emulator i already had opencloud on. |
|
Not sure 100% yet, but can it be that https://github.com/opencloud-eu/android/pull/36/files#diff-9f289054dcdd01f9adb4859d64ffff91098241a246f4778846ede49f19aeb3f2 misses the migrations for tusResumableVersion tusUploadLength tusUploadUrl tusUploadConcat tusUploadChecksum tusUploadMetadata maybe more |
|
I have fixed the crash by adding the missing migration steps for the transfers table in Migration_48.kt. I also created a test MigrationToDB48Test.kt and verified that the migration works correctly. |
4 participants
Implementierung von wiederaufnehmbaren Uploads mit dem TUS-Protokoll
Zusammenfassung
Dieses Pull-Request führt die Unterstützung für das TUS-Protokoll (v1.0.0) für Datei-Uploads ein. Dies ermöglicht robuste, wiederaufnehmbare und in Blöcke aufgeteilte Uploads, was die Zuverlässigkeit bei großen Dateien und instabilen Netzwerkverbindungen erheblich verbessert. Die Implementierung ist in das bestehende
WorkManager-System für Hintergrund-Uploads integriert und beinhaltet einen Fallback-Mechanismus auf traditionelle WebDAV-Uploads, falls TUS nicht verfügbar ist.Problembeschreibung
Das Hochladen großer Dateien auf mobilen Geräten ist anfällig für Fehler durch Netzwerkunterbrechungen, das Schließen der App oder Server-Timeouts. Der bisherige, auf WebDAV basierende Upload-Mechanismus unterstützte keine Wiederaufnahme von Uploads. Das bedeutete, dass jede Unterbrechung einen vollständigen Neustart des Uploads erforderte. Dies führte zu einer schlechten Benutzererfahrung, verschwendeter Bandbreite und einer hohen Fehlerquote bei großen Dateien.
Lösungsansatz
Dieses PR löst das Problem, indem das TUS-Protokoll als primäre Methode für Uploads implementiert wird, die eine bestimmte Chunk-Größe überschreiten.
Wesentliche Änderungen im Detail:
TUS-Protokoll-Integration:
RemoteOperation-Klassen für die zentralen TUS-Aktionen wurden hinzugefügt:Tus-Resumable,Upload-Offset, etc.) wurden in opencloudComLibrary/src/main/java/eu/opencloud/android/lib/common/http/HttpConstants.java definiert.Anpassungen der Worker-Klassen:
CHUNK_SIZEsind.opencloudDomain/src/main/java/eu/opencloud/android/domain/transfers/TransferRepository.ktin der Datenbank gespeichert. Bei einer Unterbrechung kann der Worker den Upload genau an der letzten bekannten Position fortsetzen.PATCH-Operation zum Hochladen der Chunks enthält nun einen robusten Wiederholungsmechanismus. Bei vorübergehenden Netzwerkfehlern wird der Versuch nach einer exponentiell ansteigenden Wartezeit wiederholt, um die Stabilität zu erhöhen.Datenbank-Erweiterungen:
opencloudDomain/src/main/java/eu/opencloud/android/domain/transfers/model/OCTransfer.kt-Datenmodell und dasTransferRepositorywurden um Felder zur Speicherung des TUS-Zustands erweitert (tusUploadUrl,tusUploadOffset, etc.), um die Wiederaufnahmefähigkeit über App-Neustarts hinweg zu gewährleisten.UI- und Fortschrittsanzeige:
WorkManager-API direkt aus der TUS-Upload-Schleife an die Benutzeroberfläche (opencloudApp/src/main/java/eu/opencloud/android/ui/activity/UploadListActivity.java) gemeldet. Dies sorgt für eine nahtlose und korrekte Fortschrittsanzeige.Logging:
Timberversehen, um die Fehlersuche und Überprüfung zu vereinfachen.Anleitung zum Testen
Erfolgreicher TUS-Upload:
CHUNK_SIZEist.Test der Wiederaufnahmefähigkeit:
Test des Fallback-Mechanismus:
Test bei App-Neustart: