Fix lack of ProtocolVersion in the Result.summary() (#937)

When there are more than one observer subscribed to the ResultStreamObserver, the first notified observed got the summary with the protocol info because the connection holder stills holding the connection. The next observers notified won't have the protocol version since the connection holder will not have the connection in hands at the moment.

This situation is occurring during some tests where result iterator is not fully consumed and the summary is called in the meantime.

Checking for existance of the summary in the result and notifying the observer with the already created summary should solve issue since the first summary created will have the protocol version set.

Author: bigmontz

packages/core/src/result.ts

@@ -436,7 +436,10 @@ class Result implements Promise<QueryResult> {
     const onKeysOriginal = observer.onKeys ?? DEFAULT_ON_KEYS
 
     const onCompletedWrapper = (metadata: any): void => {
-      this._createSummary(metadata).then(summary => {
+      this._releaseConnectionAndGetSummary(metadata).then(summary => {
+        if (this._summary !== null) {
+          return onCompletedOriginal.call(observer, this._summary)
+        }
         this._summary = summary
         return onCompletedOriginal.call(observer, summary)
       }).catch(onErrorOriginal)
@@ -484,7 +487,7 @@ class Result implements Promise<QueryResult> {
    * @param metadata
    * @returns
    */
-  private _createSummary (metadata: any): Promise<ResultSummary> {
+  private _releaseConnectionAndGetSummary (metadata: any): Promise<ResultSummary> {
     const {
       validatedQuery: query,
       params: parameters

packages/core/test/result.test.ts

@@ -26,6 +26,7 @@ import {
 } from '../src'
 
 import Result from '../src/result'
+import FakeConnection from './utils/connection.fake'
 
 describe('Result', () => {
   const expectedError = newError('some error')
@@ -146,11 +147,14 @@ describe('Result', () => {
           { query: 'query', parameters: { b: 2 } }
         ]
       ])('when query=%s and parameters=%s', (query, params, expected) => {
+        let connectionHolderMock: connectionHolder.ConnectionHolder
         beforeEach(() => {
+          connectionHolderMock = new connectionHolder.ConnectionHolder({})
           result = new Result(
             Promise.resolve(streamObserverMock),
             query,
-            params
+            params,
+            connectionHolderMock
           )
         })
 
@@ -174,6 +178,44 @@ describe('Result', () => {
           expect(summary).toEqual(expectedSummary)
         })
 
+        it('should resolve pre-existing summary', async () => {
+          const metadata = {
+            resultConsumedAfter: 20,
+            resultAvailableAfter: 124,
+            extraInfo: 'extra'
+          }
+          const protocolVersion = 5.0
+
+          const expectedSummary = new ResultSummary(
+            expected.query,
+            expected.parameters,
+            metadata,
+            protocolVersion
+          )
+
+          jest.spyOn(connectionHolderMock, 'getConnection')
+            .mockImplementationOnce(async () => {
+              const conn = new FakeConnection()
+              conn.protocolVersion = protocolVersion
+              return conn
+            })
+
+          jest.spyOn(connectionHolderMock, 'releaseConnection')
+            .mockImplementationOnce(async () => null)
+
+          const iterator = result[Symbol.asyncIterator]()
+          const next = iterator.next()
+          const summaryPromise = result.summary()
+
+          streamObserverMock.onCompleted(metadata)
+
+          const { value } = await next
+          const summary = await summaryPromise
+
+          expect(value).toEqual(expectedSummary)
+          expect(summary).toEqual(expectedSummary)
+        })
+
         it('should reject a pre-existing error', async () => {
           const expectedError = newError('the expected error')
           streamObserverMock.onError(expectedError)