feat: Add `WireCellsLocalAssetRepository` - WPB-18848 #3400

samwyndham · 2025-07-28T09:18:12Z

WPB-18848 [iOS] Create repository that returns local URL to assets

Issue

This PR introduces a WireCellsLocalAssetRepository. This repository is responsible for downloading and accessing files from wire cells. A WireCellsLocalAsset holds metadata about an asset (id, size, etc) and what its current download state is. The repository provides a way of observing these assets for a given ID. This will allow table view cells in the conversation to have live updates as things change - for example showing a download progress bar or updating if the name of a file is changed remotely.

Currently I'm not super happy about the implementation and may revisit this in a little when I come to using it in the project. However, the public APIs of WireCellsLocalAssetRepository and WireCellsLocalAsset are how I imagine they should be.

Testing

Run automated tests

Checklist

Title contains a reference JIRA issue number like [WPB-XXX].
Description is filled and free of optional paragraphs.
Adds/updates automated tests.

…a(nodeID:)`

github-actions · 2025-07-28T09:23:58Z

Test Results

62 tests 62 ✅ 12s ⏱️
13 suites 0 💤
1 files 0 ❌

Results for commit 10ce4b6.

♻️ This comment has been updated with latest results.

dmitrysimkin · 2025-07-28T12:17:27Z

WireCells/Sources/WireCellsAPI/Model/WireCellsLocalAsset.swift

+    /// The size of the asset in bytes as defined by the backend or `-1` if unknown.
+
+    public let size: UInt64?


question: if -1 as default, does it need to be optional?

Oh the comment is wrong here. When it gets stored in core data it will be -1. In the domain it will be optional. I will fix it. Thanks!

dmitrysimkin · 2025-07-28T12:18:24Z

WireCells/Sources/WireCellsAPI/Model/WireCellsLocalAssetMetadata.swift

+    public var cacheKey: String {
+        "\(nodeID.uuidString)-\(eTag)"
+    }


question: is it data layer or domain layer property?

I guess that whole object should just be data layer. I will move it

dmitrysimkin · 2025-07-28T12:22:16Z

WireCells/Sources/WireCellsAPI/Model/WireCellsNodeDTO.swift

question: usually I'm against DTO in the name because it's not clear to which layer it belongs and what it maps form or to?
I'd suggest some other prefix+model, e.g. StoredModel, CachedModel, NetworkModel, etc.
Is it possible to find better name for this model?

Agreed. This is still left over from the original implementation and I haven't got around to changing it yet. It should be the NetworkModel I believe. I'll fix it. Eventually it will move somewhere else.

dmitrysimkin · 2025-07-28T12:24:34Z

WireCells/Sources/WireCellsAPI/Persistence/FileCache.swift


-final class WireCellsTests: XCTestCase {}
+// sourcery: AutoMockable
+public protocol FileCache: Sendable {


Question: FileCache is about persistence but lives under WireCellsAPI which is network related code and FileCache is more like related to file system? Probably better put to some other folder?

FYI WireCellsAPI is not network related code - it's currently a big mix. It used to be a way of the different layers talking to each other. Julian is working on moving existing code from WireCells to WireMessaging after which we will follow our clean architecture approach. I will will move this to WireMessagingData once his PR is done.

WireCells/Sources/WireCellsAPI/Persistence/WireCellsLocalAssetMetadataStore.swift

dmitrysimkin · 2025-07-28T12:33:16Z

WireCells/Sources/WireCellsImplementation/WireCellsLocalAssetRepository.swift

+        switch downloadStates[nodeID] {
+        case .downloading:
+            throw WireCellsLocalAssetRepositoryError.downloadAlreadyInProgress
+        case .error:
+            setDownloadState(nodeID: nodeID, state: .none)
+        case .none:
+            break
+        }
+
+        do {
+            var (node, metadata) = try await _refreshMetadata(nodeID: nodeID)
+            let (downloadURL, eTag) = try node.downloadInfo
+
+            let (progress, download) = fileDownloader.download(from: downloadURL)
+
+            for await progress in progress {
+                setDownloadState(nodeID: nodeID, state: .downloading(progress: progress, task: download))
+            }
+
+            let (tempURL, _) = try await download.value
+            try await fileCache.saveFile(at: tempURL, key: metadata.cacheKey)
+
+            if try metadataStore.assetMetadata(nodeID: nodeID)?.eTag == eTag {
+                // downloaded file is up-to-date so update the metadata
+                metadata.isDownloaded = true
+                try metadataStore.upsertAssetMetadata(metadata)
+            } else {
+                // remove the out-of-date file
+                try await fileCache.deleteFile(forKey: metadata.cacheKey)
+            }


question: To me it looks like more of a use case not a repo, with business logic with states and api, file cache and metadata store as individual repositories, wdyt @samwyndham ?

So making the repositories simple right and moving the logic to the use case. I will have a think about it. It might well make more sense.

caldrian · 2025-08-06T07:44:30Z

WireCells/Sources/WireCellsAPI/Model/WireCellsLocalAsset.swift

+
+    public let downloadState: DownloadState
+
+    public init(


Question: will a public init be needed?

caldrian · 2025-08-06T07:51:05Z

WireCells/Sources/WireCellsAPI/Model/WireCellsLocalAsset.swift

+
+        case failed(error: any Error)
+
+        public static func == (lhs: DownloadState, rhs: DownloadState) -> Bool {


question: Do we really need Equatable conformance?
If we want to have it, I'm not sure that the current implementation, ignoring the actual error type, is behaving like any user would expect.

optional: One alternative I could think of is offering properties, like isPending: Bool etc.
But there are other possibilities, depending on what the goal actually is.

caldrian · 2025-08-06T07:55:52Z

WireCells/Sources/WireCellsAPI/Persistence/WireCellsLocalAssetMetadataStore.swift

+public import Foundation
+
+// sourcery: AutoMockable
+@MainActor


question: is it really needed to restrict every conforming type to be isolated to the @MainActor?
What about moving the attribute to the funcs?

caldrian · 2025-08-06T08:08:19Z

WireCells/Sources/WireCellsImplementation/URLSession+FileDownloading.swift

+import Foundation
+public import WireCellsAPI
+
+extension URLSession: FileDownloading {


Personal opinion: Declaring protocols and making third party (or SDK) types conform to one's own protocols is an anti-pattern for me. As usual I'm having difficulties phrasing my thoughts why it feels wrong.

But one example what I don't like is that from now on every target which links WireCells suddenly offers a URLSession.download(from:) method.
Depending on whether we want this code to be a general helper for any other part of the codebase, or whether this is something WireCells specific, we should either move this into WireUtilities/WireFoundation or prevent the namespace of URLSession from being spoiled with this API.

I suggest to make a simple structure wrapping an URLSession and conforming to the FileDownloading protocol.

caldrian · 2025-08-06T08:12:40Z

WireCells/Sources/WireCellsImplementation/URLSessionTaskProgressDelegate.swift

+    }
+
+    func urlSession(_ session: URLSession, didCreateTask task: URLSessionTask) {
+        precondition(cancellables.isEmpty, "Delegate must not be reused across multiple tasks.")


question: does it mean if one wants to download two files the following will have to be done in a strictly sequential order?

set URLSession delegate to a new instance of URLSessionTaskProgressDelegate

start a task and wait for it to be finished

replace the delegate of the URLSession

start another task

caldrian · 2025-08-06T08:14:33Z

WireCells/Sources/WireCellsImplementation/WireCellsLocalAssetRepository.swift

+import Foundation
+import WireCellsAPI
+
+/// Repository for accessing & updating `WireCellsLocalAsset`s.


Suggested change

/// Repository for accessing & updating `WireCellsLocalAsset`s.

/// Repository for accessing & updating `WireCellsLocalAssets.

Since you're the native speaker feel free to ignore this.

samwyndham added 15 commits July 25, 2025 10:54

Add FileDownloading protocol + implementation

1b3f1d0

Update FileDownloading protocol shuffle files

af47396

Delete empty test file

84d5661

Test URLSessionTaskProgressDelegate

07bb65d

Add downloadURL to WireCellsNode

00f1fb8

Add WireCellsLocalAssetRepository

0583d6f

Test WireCellsLocalAssetRepository.asset(nodeID:)

6a447aa

Test WireCellsLocalAssetRepository.refreshMetadata(nodeID:)

ebd45a4

Small refactor of tests

f469ee8

Test WireCellsLocalAssetRepository.downloadAsset(nodeID:)

4fb8f1d

Lint & format

bdcdf8d

Add doc comments to WireCellsLocalAssetRepository

9368630

Don't return value from `WireCellsLocalAssetRepository.refreshMetadat…

9c3b3b3

…a(nodeID:)`

Use UInt64 for size

efb7c7f

Add doc comments to WireCellsLocalAsset

b076a18

github-actions bot assigned samwyndham Jul 28, 2025

Fix linter warning

10ce4b6

samwyndham requested review from a team, netbe, caldrian and KaterinaWire and removed request for a team and netbe July 28, 2025 09:34

dmitrysimkin reviewed Jul 28, 2025

View reviewed changes

caldrian approved these changes Aug 6, 2025

View reviewed changes

		/// The size of the asset in bytes as defined by the backend or `-1` if unknown.

		public let size: UInt64?


		case failed(error: any Error)

		public static func == (lhs: DownloadState, rhs: DownloadState) -> Bool {

	/// Repository for accessing & updating `WireCellsLocalAsset`s.
	/// Repository for accessing & updating `WireCellsLocalAssets.

feat: Add WireCellsLocalAssetRepository - WPB-18848 #3400

Are you sure you want to change the base?

feat: Add WireCellsLocalAssetRepository - WPB-18848 #3400

Uh oh!

Conversation

samwyndham commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Testing

Checklist

Uh oh!

github-actions bot commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caldrian Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat: Add `WireCellsLocalAssetRepository` - WPB-18848 #3400

feat: Add `WireCellsLocalAssetRepository` - WPB-18848 #3400

samwyndham commented Jul 28, 2025 •

edited

Loading

github-actions bot commented Jul 28, 2025 •

edited

Loading

caldrian Aug 6, 2025 •

edited

Loading