fix(sql_connect): queries are not single instanced for execute

[aashishpatil-g]

QueryRefs are not single instanced when calling execute so queryrefs get overwritten when first subscribe is called resulting in orphaned queryrefs created for execute.

[gemini-code-assist]

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

[aashishpatil-g]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements query tracking in FirebaseDataConnect to ensure that identical queries return the same QueryRef instance. It updates QueryRef to clean up its _streamController reference when closed, stores newly created queries in _queryManager.trackedQueries, and adds a unit test to verify this caching behavior. There are no review comments, and I have no feedback to provide.

[dconeybe]

Hi @aashishpatil-g. The code LGTM but I ran it through the code review skill and it made some suggestions that I confirmed (to the extent possible, as I am not a dart/flutter expert). All of the comments were written by AI but are consistent with my understanding. Feel free to defer the fixes to a later PR if you see fit, but IIUC they should proabably be addressed before introducing memory leaks and/or the possibility of duplicate streams, which would cause the connection to abruptly get terminated by the backend.

[dconeybe]

Memory leak due to untracked QueryRef lifecycle.

Adding newRef to trackedQueries here means it will stay there forever if subscribe() is never called, as there is no mechanism to remove it other than when all subscribers cancel (which never happens if they never subscribe).

To fix this, we should use WeakReference to track queries.

Note: You will also need to update the lookup at lines 142-143 to retrieve the target from the WeakReference:

    final weakRef = _queryManager.trackedQueries[queryId];
    QueryRef<Data, Variables>? ref =
        (weakRef as WeakReference<QueryRef<Data, Variables>>?)?.target;

Suggested change

	_queryManager.trackedQueries[queryId] = newRef;
	return newRef;
	_queryManager.trackedQueries[queryId] = WeakReference(newRef);
	return newRef;

[dconeybe]

Inconsistent QueryRef identity and potential duplicate server streams.

When all subscribers to a query cancel, the QueryRef is currently removed from trackedQueries (via the stream controller's onCancel). If query() is subsequently called again for the same operation, a new QueryRef instance will be created. If the developer still holds the old QueryRef and resubscribes to it, both the old and new instances will be active, leading to multiple active server streams for the same logical query.

To fix this and maintain a single canonical instance, QueryManager should use WeakReference to track queries, allowing them to be reused if they still exist in memory, while avoiding leaks.

Please apply the following changes to QueryManager (not shown in this diff):

1. Change trackedQueries type to use WeakReference:

final Map<String, WeakReference<QueryRef<dynamic, dynamic>>> trackedQueries = {};

2. Update addQuery to store WeakReference and NOT remove it on cancel:

  StreamController<QueryResult<Data, Variables>> addQuery<Data, Variables>(
    QueryRef<Data, Variables> ref,
  ) {
    final queryId = ref.operationId;
    trackedQueries[queryId] = WeakReference(ref);

    final streamController =
        StreamController<QueryResult<Data, Variables>>.broadcast(
      onCancel: () {
        // Do NOT remove from trackedQueries here.
        // Let WeakReference handle cleanup when the ref is GCed.
        ref._onAllSubscribersCancelled();
      },
    );

    return streamController;
  }

3. Update QueryRef.subscribe() to store WeakReference:

  Stream<QueryResult<Data, Variables>> subscribe() {
    _streamController ??= _queryManager.addQuery(this);
    // ...
    _queryManager.trackedQueries[operationId] = WeakReference(this);
    // ...

[dconeybe]

Broadcast StreamController is not closed.

In _onAllSubscribersCancelled, _streamController is set to null but the controller itself is not closed. Although it may eventually be garbage collected once all references are dropped, it is standard best practice to explicitly close StreamControllers to release resources immediately.

Please update QueryManager.addQuery (not in this diff) to close the controller in onCancel:

    final streamController =
        StreamController<QueryResult<Data, Variables>>.broadcast(
      onCancel: () {
        trackedQueries.remove(queryId); // Or keep it if using WeakReference
        ref._onAllSubscribersCancelled();
        streamController.close(); // Close the controller
      },
    );

[dconeybe]

Here is a reproduction test case that verifies the QueryRef identity is preserved even after all subscriptions are cancelled (as long as the application still holds a reference to the QueryRef).

Without the WeakReference fix, this test fails because unsubscribe removes the query from trackedQueries, causing subsequent query() calls to return a new instance.

You can append this test to the query group:

Suggested change

	});
	});
	test('query returns identical QueryRef instance even after unsubscribe if still referenced', () async {
	final dynamicApp = DynamicMockFirebaseApp(
	name: 'queryRefAppName',
	options: const FirebaseOptions(
	apiKey: 'fake_api_key',
	appId: 'fake_app_id',
	messagingSenderId: 'fake_messaging_sender_id',
	projectId: 'fake_project_id',
	),
	);
	final instance = FirebaseDataConnect(
	app: dynamicApp,
	connectorConfig: mockConnectorConfig,
	);
	final ref1 = instance.query(
	'listMovies',
	(json) => json,
	emptySerializer,
	null,
	);
	final subscription = ref1.subscribe().listen((_) {});
	await subscription.cancel();
	final ref2 = instance.query(
	'listMovies',
	(json) => json,
	emptySerializer,
	null,
	);
	expect(identical(ref1, ref2), isTrue);
	});