Cache implementation #68

aashishpatil-g · 2025-11-11T19:36:40Z

Implement caching in base (core) SDK

For googlers, refer to the doc go/fdc-sdk-caching

TODO - needs cleanup

…ion info

…and EntityNode

dconeybe · 2025-11-12T15:53:29Z

/gemini review

aashishpatil-g · 2025-11-12T23:52:08Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a comprehensive caching layer to the SDK, a significant and valuable feature. The implementation includes support for both in-memory and persistent storage via SQLite, data normalization to reduce redundancy, and automatic cache invalidation on authentication state changes. The overall architecture is well-thought-out. However, the review has identified several critical and high-severity issues that must be addressed. These include SQL injection vulnerabilities in the SQLite provider, multiple potential crash scenarios due to force-unwrapping optionals, and a critical performance issue from a hardcoded sleep. There are also other bugs and areas for improvement in robustness and code cleanliness. Please address these points to ensure the stability, security, and performance of the new caching feature.

Sources/Cache/SQLiteCacheProvider.swift

Sources/Queries/GenericQueryRef.swift

Sources/Cache/Cache.swift

Sources/Cache/SQLiteCacheProvider.swift

Sources/Cache/ResultTreeProcessor.swift

Sources/Queries/GenericQueryRef.swift

Sources/Cache/EntityNode.swift

Sources/Cache/EntityDataObject.swift

gemini-code-assist · 2025-11-12T23:55:14Z

Sources/Internal/GrpcClient.swift

+       */
+      guard !errorInfoList.isEmpty else {
+        // TODO: Extract ttl, server timestamp
+        return ServerResponse(jsonResults: resultsString, ttl: 10.0)


The ttl (Time To Live) for the server response is hardcoded to 10.0. The TODO comment indicates this is temporary. This value should be extracted from the server response or the service configuration to allow for flexible and correct caching behavior, rather than being a fixed constant.

This will be resolved once we finalize on ttl location

Sources/Cache/Cache.swift

ncooke3 · 2025-11-17T21:06:26Z

Sources/OperationResult.swift

+
+import Foundation
+
+/// Struct returned by operation calls - query or mutation


I've read in Apple's docs that they will refer to structs in doc comments as "structure" or "value".

What about something like this?

Suggested change

/// Struct returned by operation calls - query or mutation

/// The value returned by query or mutation operations.

ncooke3 · 2025-11-17T21:21:50Z

Sources/MutationRef.swift

+
+  public static func == (lhs: MutationRef<ResultData, Variable>,
+                         rhs: MutationRef<ResultData, Variable>) -> Bool {
+    return lhs.request as? MutationRequest<Variable> == rhs.request as? MutationRequest<Variable>


There is one possible edge case where if two MutationRefs with different, non-MutationRequest-typed requests are compared, the comparison will be true.

Would it make sense to change the request property's type from any OperationRequest to MutationRequest<Variable>?

public class MutationRef< ResultData: Decodable & Sendable, Variable: OperationVariable >: OperationRef { private var request: MutationRequest<Variable> // ✨

Doing so would clean up the need for optional downcasting:

Suggested change

return lhs.request as? MutationRequest<Variable> == rhs.request as? MutationRequest<Variable>

return lhs.request == rhs.request

I think the suggestion is worth it if a MutationRef maps 1:1 with a MutationRequest.

ncooke3 · 2025-11-17T21:25:41Z

Tests/Unit/CacheTests.swift

+  // Confirm that providing same entity cache id uses the same EntityDataObject instance
+  func testEntityDataObjectReuse() throws {
+    do {
+      guard let cacheProvider else {


optional nit: The guard here and in the other test case could be simplified with try XCTUnwrap(cacheProvider) or making the proerty implicitly non-optional with a !.

ncooke3 · 2025-11-17T21:26:35Z

Tests/Unit/CacheTests.swift

+      let user2 = cacheProvider.entityData(reused_id)
+
+      // both user objects should be references to same instance
+      XCTAssertTrue(user1 == user2)


Given above comment, I think this should be:

Suggested change

XCTAssertTrue(user1 == user2)

XCTAssertTrue(user1 === user2)

ncooke3 · 2025-11-17T21:29:30Z

Sources/Scalars/AnyValue.swift


 import Foundation

+import FirebaseCore


Is import needed anymore, given the code removal?

ncooke3 · 2025-11-17T21:31:50Z

Sources/Queries/QueryRequest.swift

+      return false
+    }
+
+    if lhs.variables == nil && rhs.variables == nil {


nil == nil == true so couldn't the here and below be replaced with:

return lhs.variables == rhs.variables

couldn't ~~the~~ here

ncooke3 · 2025-11-17T21:34:01Z

Sources/Queries/QueryRequest.swift

+    self.variables = variables
+  }
+
+  // Hashable and Equatable implementation


optional nit:

Suggested change

// Hashable and Equatable implementation

// MARK: - Hashable and Equatable

ncooke3 · 2025-11-17T21:36:29Z

Sources/Queries/QueryRequest.swift

+@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
+struct QueryRequest<Variable: OperationVariable>: OperationRequest, Hashable, Equatable {
+  private(set) var operationName: String
+  private(set) var variables: Variable?


This is minor, but I would think the variables would map to an array of Variable (e.g. [Variable])?

ncooke3 · 2025-11-17T21:39:40Z

Sources/Queries/QueryRef.swift

+import CryptoKit
+import Foundation
+
+import Firebase


This import looks unused.

ncooke3 · 2025-11-17T21:42:08Z

Sources/Queries/ObservableQueryRef.swift

 // See the License for the specific language governing permissions and
 // limitations under the License.

+import CryptoKit


This import looks unused.

Suggested change

import CryptoKit

ncooke3 · 2025-11-17T21:42:16Z

Sources/Queries/ObservableQueryRef.swift

+import CryptoKit
 import Foundation

+import Firebase


This import looks unused.

Suggested change

import Firebase

ncooke3 · 2025-11-17T21:47:19Z

Sources/Queries/ObservableQueryRef.swift

 /// - ``lastError``: Published variable that contains ``DataConnectError``  if last fetch had error.
 ///            If last fetch was successful, this variable is cleared
-@available(macOS 14, iOS 17, tvOS 17, watchOS 10, *)
+@available(macOS 15, iOS 17, tvOS 17, watchOS 10, *)


Why the macOS version bump? The Observable macro looks to be available on macOS 14?

https://developer.apple.com/documentation/observation/observable

ncooke3 · 2025-11-17T21:48:14Z

Sources/Queries/ObservableQueryRef.swift

  public private(set) var lastError: DataConnectError?

+  /// Source of the query results (server, local cache, ...)
+  public private(set) var source: DataSource?


Why is this mutable again? Is it because the contents of this class will change each time an update is observed?

ncooke3 · 2025-11-17T21:49:16Z

Sources/Queries/GenericQueryRef.swift

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import CryptoKit


nit: looks unused

Suggested change

import CryptoKit

ncooke3 · 2025-11-17T21:49:24Z

Sources/Queries/GenericQueryRef.swift

+import CryptoKit
+import Foundation
+
+import Firebase


nit: looks unused

Suggested change

import Firebase

ncooke3 · 2025-11-17T21:49:55Z

Sources/Queries/GenericQueryRef.swift

+
+  private let cache: Cache?
+
+  private var ttl: TimeInterval? = 10.0 //


Looks like a comment was intended? Also, does this need to be optional?

ncooke3 · 2025-11-17T21:53:12Z

Sources/Internal/OperationsManager.swift


      if publisher == .auto || publisher == .observableMacro {
-        if #available(iOS 17, macOS 14, tvOS 17, watchOS 10, *) {
+        if #available(iOS 17, macOS 15, tvOS 17, watchOS 10, *) {


Why the macOS bump?

https://developer.apple.com/documentation/observation/observable

ncooke3 · 2025-11-17T21:57:06Z

Sources/DataConnect.swift


-      operationsManager = OperationsManager(grpcClient: grpcClient)
+      if let cache {
+        self.cache = Cache(config: cache.config, dataConnect: self)


Won't there be a strong retain cycle between Cache and DataConnect?

Maybe Cache only needs to retain immutable data from the DC instance. Or the Cache weakly retains DC...

ncooke3 · 2025-11-17T23:06:04Z

Sources/Cache/CacheSettings.swift

+  public let storage: Storage // default provider is persistent type
+  public let maxSizeBytes: UInt64
+
+  public init(storage: Storage = .persistent, maxSize: UInt64 = 100_000_000) {


suggestion: Maybe generate a doc comment for this method.

ncooke3 · 2025-11-17T23:08:09Z

Sources/Cache/ResultTree.swift

+
+import Foundation
+
+import FirebaseCore


nit: unused import

Suggested change

import FirebaseCore

ncooke3 · 2025-11-17T23:08:51Z

Sources/Cache/ResultTree.swift

+
+@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
+struct ResultTree {
+  let data: String // tree data - could be hydrated or dehydrated.


optional nit: generally, i would put comments above the lines so they are more discoverable

ncooke3 · 2025-11-17T23:09:59Z

Sources/Cache/ResultTree.swift

+  }
+}
+
+extension ResultTree: Codable {


What is custom here? Why not use the generated Codable impl?

aashishpatil-g added 25 commits October 29, 2025 09:19

Initial Stub Classes

7c92921

Wire up Initialization of CacheProvider

f56a8cc

query level caching

f655a15

TODO - needs cleanup

Cache Normalization Implementation

90c7ae1

Persistent Provider

5174e03

Add last_accessed field

ad71e3c

Uber Cache object, support Auth uid scope and Refactor classes

c0a024b

Implement operationId on QueryRef

0bcdf09

Fix sha256 formatter.

ad749e7

Accumulate Impacted Refs and Reload Local

35dc280

Update construction of cacheIdentifier to include connector and locat…

6cd60ef

…ion info

Use inmemory SQLite for Ephemeral cache provider

824a41e

Minor updates to API

f503f5e

Refactor BackingDataObject, STubDataObject names to EntityDataObject …

e57bbb4

…and EntityNode

DispatchQueue for EntityDataObject access

353caf5

Externalize table and column name strings into constants

78599a3

API Review feedback

250e905

Code formatting updates

f1aa717

Refactor name of OperationResultSource

d9b4807

API feedback - rename maxSize => maxSizeBytes

1c666ec

API Council Review Feedback

8ea6762

Move ServerResponse

a807d9f

Update cache type to match API review feedback

336e41b

Cleanup and minor fixes to docs

37493c4

Update globalID value

dd8569c

aashishpatil-g requested review from dconeybe, maneesht, ncooke3 and stephenarosaj November 11, 2025 19:36

aashishpatil-g added 2 commits November 12, 2025 15:24

Fix @available

30940a6

fix integration tests

5482987

gemini-code-assist bot reviewed Nov 12, 2025

View reviewed changes

aashishpatil-g added 4 commits November 12, 2025 22:37

Gemini Review part 1

1a9aeb9

formatting fixes

12da757

Convert sync queue calls to actor

334e710

comments

b1569e8

ncooke3 reviewed Nov 17, 2025

View reviewed changes

Sources/Scalars/AnyValue.swift

import Foundation

import FirebaseCore

Copy link

Member

ncooke3 Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is import needed anymore, given the code removal?

ncooke3 reviewed Nov 17, 2025

View reviewed changes

Sources/Queries/QueryRef.swift

import CryptoKit

import Foundation

import Firebase

Copy link

Member

ncooke3 Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import looks unused.

ncooke3 reviewed Nov 17, 2025

View reviewed changes

Sources/Cache/ResultTree.swift

import Foundation

import FirebaseCore

Copy link

Member

ncooke3 Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unused import

Suggested change

import FirebaseCore

ncooke3 reviewed Nov 17, 2025

View reviewed changes

Sources/Cache/ResultTree.swift

}

}

extension ResultTree: Codable {

Copy link

Member

ncooke3 Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is custom here? Why not use the generated Codable impl?


		import Foundation

		/// Struct returned by operation calls - query or mutation

	/// Struct returned by operation calls - query or mutation
	/// The value returned by query or mutation operations.

	return lhs.request as? MutationRequest<Variable> == rhs.request as? MutationRequest<Variable>
	return lhs.request == rhs.request

	// Hashable and Equatable implementation
	// MARK: - Hashable and Equatable


		private let cache: Cache?

		private var ttl: TimeInterval? = 10.0 //

Cache implementation #68

Are you sure you want to change the base?

Cache implementation #68

Uh oh!

Conversation

aashishpatil-g commented Nov 11, 2025

Uh oh!

dconeybe commented Nov 12, 2025

Uh oh!

aashishpatil-g commented Nov 12, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot Nov 12, 2025

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!

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!

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!

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!

Reviewers

Assignees

Labels