From f615f53979c5489938315d9f2ad7d691c8c99238 Mon Sep 17 00:00:00 2001 From: Alka Trivedi Date: Mon, 13 Jan 2025 12:04:33 +0530 Subject: [PATCH] refactor --- observability-test/database.ts | 1497 ++++++++++++-------------------- src/database.ts | 38 +- src/session-factory.ts | 2 +- test/database.ts | 206 +---- test/spanner.ts | 2 +- 5 files changed, 637 insertions(+), 1108 deletions(-) diff --git a/observability-test/database.ts b/observability-test/database.ts index 3ce6e73ee..935b35d9a 100644 --- a/observability-test/database.ts +++ b/observability-test/database.ts @@ -47,7 +47,7 @@ import {Instance, MutationGroup, Spanner} from '../src'; import * as pfy from '@google-cloud/promisify'; import {grpc} from 'google-gax'; import {MockError} from '../test/mockserver/mockspanner'; -import {FakeMultiplexedSession, FakeSessionFactory} from '../test/database'; +import {FakeSessionFactory} from '../test/database'; const {generateWithAllSpansHaveDBName} = require('./helper'); const fakePfy = extend({}, pfy, { @@ -505,310 +505,196 @@ describe('Database', () => { describe('getSnapshot', () => { let fakeSessionFactory: FakeSessionFactory; let fakeSession: FakeSession; - let fakeMultiplexedSession: FakeMultiplexedSession; let fakeSnapshot: FakeTransaction; let beginSnapshotStub: sinon.SinonStub; let getSessionStub: sinon.SinonStub; - const muxEnabled = [true, false]; - - muxEnabled.forEach(isMuxEnabled => { - describe( - 'when GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION is ' + - `${isMuxEnabled ? 'enabled' : 'disable'}`, - () => { - before(() => { - isMuxEnabled - ? (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION = 'true') - : (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION = - 'false'); - }); + beforeEach(() => { + fakeSessionFactory = database.sessionFactory_; + fakeSession = new FakeSession(); + fakeSnapshot = new FakeTransaction( + {} as google.spanner.v1.TransactionOptions.ReadOnly + ); - beforeEach(() => { - fakeSessionFactory = database.sessionFactory_; - fakeSnapshot = new FakeTransaction( - {} as google.spanner.v1.TransactionOptions.ReadOnly - ); - beginSnapshotStub = ( - sandbox.stub(fakeSnapshot, 'begin') as sinon.SinonStub - ).callsFake(callback => callback(null)); - - if (isMuxEnabled) { - fakeMultiplexedSession = new FakeMultiplexedSession(); - getSessionStub = ( - sandbox.stub( - fakeSessionFactory, - 'getSession' - ) as sinon.SinonStub - ).callsFake(callback => callback(null, fakeMultiplexedSession)); - ( - sandbox.stub( - fakeMultiplexedSession, - 'snapshot' - ) as sinon.SinonStub - ).returns(fakeSnapshot); - - sandbox - .stub(fakeSessionFactory, 'isMultiplexedEnabled') - .returns(true); - } else { - fakeSession = new FakeSession(); - getSessionStub = ( - sandbox.stub( - fakeSessionFactory, - 'getSession' - ) as sinon.SinonStub - ).callsFake(callback => callback(null, fakeSession)); - - sandbox.stub(fakeSession, 'snapshot').returns(fakeSnapshot); - - sandbox - .stub(fakeSessionFactory, 'isMultiplexedEnabled') - .returns(false); - } - }); + beginSnapshotStub = ( + sandbox.stub(fakeSnapshot, 'begin') as sinon.SinonStub + ).callsFake(callback => callback(null)); - it('with error', done => { - const fakeError = new Error('our snapshot error'); - - getSessionStub.callsFake(callback => callback(fakeError, null)); - - database.getSnapshot(err => { - assert.strictEqual(err, fakeError); - traceExporter.forceFlush(); - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); - withAllSpansHaveDBName(spans); - - const actualSpanNames: string[] = []; - const actualEventNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = ['CloudSpanner.Database.getSnapshot']; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const firstSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.ERROR, - firstSpan.status.code, - 'Expected an ERROR span status' - ); - assert.strictEqual( - 'our snapshot error', - firstSpan.status.message, - 'Mismatched span status message' - ); - - // We don't expect events. - const expectedEventNames = []; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - }); + getSessionStub = ( + sandbox.stub(fakeSessionFactory, 'getSession') as sinon.SinonStub + ).callsFake(callback => callback(null, fakeSession)); + + sandbox.stub(fakeSession, 'snapshot').returns(fakeSnapshot); + + sandbox.stub(fakeSessionFactory, 'isMultiplexedEnabled').returns(false); + }); + + it('with error', done => { + const fakeError = new Error('our snapshot error'); + + getSessionStub.callsFake(callback => callback(fakeError, null)); + + database.getSnapshot(err => { + assert.strictEqual(err, fakeError); + traceExporter.forceFlush(); + const spans = traceExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); + withAllSpansHaveDBName(spans); + + const actualSpanNames: string[] = []; + const actualEventNames: string[] = []; + spans.forEach(span => { + actualSpanNames.push(span.name); + span.events.forEach(event => { + actualEventNames.push(event.name); }); + }); - if (isMuxEnabled) { - it('should not retry on `begin` errors with `Session not found`', done => { - const fakeError = { - code: grpc.status.NOT_FOUND, - message: 'Session not found', - } as MockError; - - getSessionStub - .onFirstCall() - .callsFake(callback => callback(null, fakeMultiplexedSession)); - - beginSnapshotStub.callsFake(callback => callback(fakeError)); - - database.getSnapshot((err, _) => { - assert.strictEqual(err, fakeError); - - provider.forceFlush(); - traceExporter.forceFlush(); - const spans = traceExporter.getFinishedSpans(); - withAllSpansHaveDBName(spans); - - assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); - - const actualSpanNames: string[] = []; - const actualEventNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = ['CloudSpanner.Database.getSnapshot']; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the first span actually produced an error that was recorded. - const parentSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.ERROR, - parentSpan.status.code, - 'Expected an ERROR span status' - ); - assert.strictEqual( - 'Session not found', - parentSpan.status.message.toString(), - 'Mismatched span status message' - ); - assert.ok( - parentSpan.spanContext().traceId, - 'Expected that the initial parent span has a defined traceId' - ); - assert.ok( - parentSpan.spanContext().spanId, - 'Expected that the initial parent span has a defined spanId' - ); - - const expectedEventNames = ['No session available']; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - }); - }); - } else { - it('with retries on `begin` errors with `Session not found`', done => { - const fakeError = { - code: grpc.status.NOT_FOUND, - message: 'Session not found', - } as MockError; - - const fakeSession2 = new FakeSession(); - const fakeSnapshot2 = new FakeTransaction( - {} as google.spanner.v1.TransactionOptions.ReadOnly - ); - ( - sandbox.stub(fakeSnapshot2, 'begin') as sinon.SinonStub - ).callsFake(callback => callback(null)); - sandbox.stub(fakeSession2, 'snapshot').returns(fakeSnapshot2); - - getSessionStub - .onFirstCall() - .callsFake(callback => callback(null, fakeSession)) - .onSecondCall() - .callsFake(callback => callback(null, fakeSession2)); - beginSnapshotStub.callsFake(callback => callback(fakeError)); - - // The first session that was not found should be released back into the - // pool, so that the pool can remove it from its inventory. - const releaseStub = sandbox.stub(fakeSessionFactory, 'release'); - - database.getSnapshot(async (err, snapshot) => { - assert.ifError(err); - assert.strictEqual(snapshot, fakeSnapshot2); - // The first session that error should already have been released back - // to the pool. - assert.strictEqual(releaseStub.callCount, 1); - // Ending the valid snapshot will release its session back into the - // pool. - snapshot.emit('end'); - assert.strictEqual(releaseStub.callCount, 2); - - await provider.forceFlush(); - await traceExporter.forceFlush(); - const spans = traceExporter.getFinishedSpans(); - withAllSpansHaveDBName(spans); - - const actualSpanNames: string[] = []; - const actualEventNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = [ - 'CloudSpanner.Database.getSnapshot', - 'CloudSpanner.Database.getSnapshot', - ]; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the first span actually produced an error that was recorded. - const parentSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.ERROR, - parentSpan.status.code, - 'Expected an ERROR span status' - ); - assert.strictEqual( - 'Session not found', - parentSpan.status.message.toString(), - 'Mismatched span status message' - ); - - // Ensure that the second span is a child of the first span. - const secondRetrySpan = spans[1]; - assert.ok( - parentSpan.spanContext().traceId, - 'Expected that the initial parent span has a defined traceId' - ); - assert.ok( - secondRetrySpan.spanContext().traceId, - 'Expected that the second retry span has a defined traceId' - ); - assert.deepStrictEqual( - parentSpan.spanContext().traceId, - secondRetrySpan.spanContext().traceId, - 'Expected that both spans share a traceId' - ); - assert.ok( - parentSpan.spanContext().spanId, - 'Expected that the initial parent span has a defined spanId' - ); - assert.ok( - secondRetrySpan.spanContext().spanId, - 'Expected that the second retry span has a defined spanId' - ); - assert.deepStrictEqual( - secondRetrySpan.parentSpanId, - parentSpan.spanContext().spanId, - 'Expected that secondRetrySpan is the child to parentSpan' - ); - - const expectedEventNames = ['No session available']; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - }); - }); - } - } + const expectedSpanNames = ['CloudSpanner.Database.getSnapshot']; + assert.deepStrictEqual( + actualSpanNames, + expectedSpanNames, + `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` + ); + + // Ensure that the span actually produced an error that was recorded. + const firstSpan = spans[0]; + assert.strictEqual( + SpanStatusCode.ERROR, + firstSpan.status.code, + 'Expected an ERROR span status' + ); + assert.strictEqual( + 'our snapshot error', + firstSpan.status.message, + 'Mismatched span status message' + ); + + // We don't expect events. + const expectedEventNames = []; + assert.deepStrictEqual( + actualEventNames, + expectedEventNames, + `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` + ); + + done(); + }); + }); + + it('with retries on `begin` errors with `Session not found`', done => { + const fakeError = { + code: grpc.status.NOT_FOUND, + message: 'Session not found', + } as MockError; + + const fakeSession2 = new FakeSession(); + const fakeSnapshot2 = new FakeTransaction( + {} as google.spanner.v1.TransactionOptions.ReadOnly + ); + (sandbox.stub(fakeSnapshot2, 'begin') as sinon.SinonStub).callsFake( + callback => callback(null) ); + sandbox.stub(fakeSession2, 'snapshot').returns(fakeSnapshot2); + + getSessionStub + .onFirstCall() + .callsFake(callback => callback(null, fakeSession)) + .onSecondCall() + .callsFake(callback => callback(null, fakeSession2)); + beginSnapshotStub.callsFake(callback => callback(fakeError)); + + // The first session that was not found should be released back into the + // pool, so that the pool can remove it from its inventory. + const releaseStub = sandbox.stub(fakeSessionFactory, 'release'); + + database.getSnapshot(async (err, snapshot) => { + assert.ifError(err); + assert.strictEqual(snapshot, fakeSnapshot2); + // The first session that error should already have been released back + // to the pool. + assert.strictEqual(releaseStub.callCount, 1); + // Ending the valid snapshot will release its session back into the + // pool. + snapshot.emit('end'); + assert.strictEqual(releaseStub.callCount, 2); + + await provider.forceFlush(); + await traceExporter.forceFlush(); + const spans = traceExporter.getFinishedSpans(); + withAllSpansHaveDBName(spans); + + const actualSpanNames: string[] = []; + const actualEventNames: string[] = []; + spans.forEach(span => { + actualSpanNames.push(span.name); + span.events.forEach(event => { + actualEventNames.push(event.name); + }); + }); + + const expectedSpanNames = [ + 'CloudSpanner.Database.getSnapshot', + 'CloudSpanner.Database.getSnapshot', + ]; + assert.deepStrictEqual( + actualSpanNames, + expectedSpanNames, + `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` + ); + + // Ensure that the first span actually produced an error that was recorded. + const parentSpan = spans[0]; + assert.strictEqual( + SpanStatusCode.ERROR, + parentSpan.status.code, + 'Expected an ERROR span status' + ); + assert.strictEqual( + 'Session not found', + parentSpan.status.message.toString(), + 'Mismatched span status message' + ); + + // Ensure that the second span is a child of the first span. + const secondRetrySpan = spans[1]; + assert.ok( + parentSpan.spanContext().traceId, + 'Expected that the initial parent span has a defined traceId' + ); + assert.ok( + secondRetrySpan.spanContext().traceId, + 'Expected that the second retry span has a defined traceId' + ); + assert.deepStrictEqual( + parentSpan.spanContext().traceId, + secondRetrySpan.spanContext().traceId, + 'Expected that both spans share a traceId' + ); + assert.ok( + parentSpan.spanContext().spanId, + 'Expected that the initial parent span has a defined spanId' + ); + assert.ok( + secondRetrySpan.spanContext().spanId, + 'Expected that the second retry span has a defined spanId' + ); + assert.deepStrictEqual( + secondRetrySpan.parentSpanId, + parentSpan.spanContext().spanId, + 'Expected that secondRetrySpan is the child to parentSpan' + ); + + const expectedEventNames = ['No session available']; + assert.deepStrictEqual( + actualEventNames, + expectedEventNames, + `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` + ); + + done(); + }); }); }); @@ -1160,7 +1046,6 @@ describe('Database', () => { }); const SESSION = new FakeSession(); - const MULTIPLEXEDSESSION = new FakeMultiplexedSession(); const RESPONSE = {commitTimestamp: {seconds: 1, nanos: 0}}; const TRANSACTION = new FakeTransaction( {} as google.spanner.v1.TransactionOptions.ReadWrite @@ -1168,283 +1053,184 @@ describe('Database', () => { let sessionFactory: FakeSessionFactory; - const muxEnabled = [true, false]; - - muxEnabled.forEach(isMuxEnabled => { - describe( - 'when GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION is ' + - `${isMuxEnabled ? 'enabled' : 'disable'}`, - () => { - before(() => { - isMuxEnabled - ? (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION = 'true') - : (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION = - 'false'); - }); - beforeEach(() => { - sessionFactory = database.sessionFactory_; - if (isMuxEnabled) { - ( - sandbox.stub(sessionFactory, 'getSession') as sinon.SinonStub - ).callsFake(callback => { - callback(null, MULTIPLEXEDSESSION, TRANSACTION); - }); - sandbox - .stub(sessionFactory, 'isMultiplexedEnabled') - .returns(true); - } else { - ( - sandbox.stub(sessionFactory, 'getSession') as sinon.SinonStub - ).callsFake(callback => { - callback(null, SESSION, TRANSACTION); - }); - sandbox - .stub(sessionFactory, 'isMultiplexedEnabled') - .returns(false); - } + beforeEach(() => { + sessionFactory = database.sessionFactory_; + (sandbox.stub(sessionFactory, 'getSession') as sinon.SinonStub).callsFake( + callback => { + callback(null, SESSION, TRANSACTION); + } + ); + sandbox.stub(sessionFactory, 'isMultiplexedEnabled').returns(false); + }); + + it('should return any errors getting a session', done => { + const fakeErr = new Error('getting session error'); + + (sessionFactory.getSession as sinon.SinonStub).callsFake(callback => + callback(fakeErr, null, null) + ); + + database.writeAtLeastOnce(mutations, err => { + assert.deepStrictEqual(err, fakeErr); + + const spans = traceExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); + withAllSpansHaveDBName(spans); + + const actualEventNames: string[] = []; + const actualSpanNames: string[] = []; + spans.forEach(span => { + actualSpanNames.push(span.name); + span.events.forEach(event => { + actualEventNames.push(event.name); }); + }); + + const expectedSpanNames = ['CloudSpanner.Database.writeAtLeastOnce']; + assert.deepStrictEqual( + actualSpanNames, + expectedSpanNames, + `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` + ); + + // Ensure that the span actually produced an error that was recorded. + const firstSpan = spans[0]; + assert.strictEqual( + SpanStatusCode.ERROR, + firstSpan.status.code, + 'Expected an ERROR span status' + ); + assert.strictEqual( + 'getting session error', + firstSpan.status.message, + 'Mismatched span status message' + ); + + // We don't expect events. + const expectedEventNames = []; + assert.deepStrictEqual( + actualEventNames, + expectedEventNames, + `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` + ); - it('should return any errors getting a session', done => { - const fakeErr = new Error('getting session error'); - - (sessionFactory.getSession as sinon.SinonStub).callsFake(callback => - callback(fakeErr, null, null) - ); - - database.writeAtLeastOnce(mutations, err => { - assert.deepStrictEqual(err, fakeErr); - - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); - withAllSpansHaveDBName(spans); - - const actualEventNames: string[] = []; - const actualSpanNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = [ - 'CloudSpanner.Database.writeAtLeastOnce', - ]; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const firstSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.ERROR, - firstSpan.status.code, - 'Expected an ERROR span status' - ); - assert.strictEqual( - 'getting session error', - firstSpan.status.message, - 'Mismatched span status message' - ); - - // We don't expect events. - const expectedEventNames = []; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); + done(); + }); + }); + + it('with empty mutation should return successful CommitResponse', done => { + const fakeMutations = new MutationSet(); + try { + database.writeAtLeastOnce(fakeMutations, (err, response) => { + assert.ifError(err); + assert.deepStrictEqual( + response.commitTimestamp, + RESPONSE.commitTimestamp + ); + + const spans = traceExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); + withAllSpansHaveDBName(spans); + + const actualEventNames: string[] = []; + const actualSpanNames: string[] = []; + spans.forEach(span => { + actualSpanNames.push(span.name); + span.events.forEach(event => { + actualEventNames.push(event.name); }); }); - it('with empty mutation should return successful CommitResponse', done => { - const fakeMutations = new MutationSet(); - try { - database.writeAtLeastOnce(fakeMutations, (err, response) => { - assert.ifError(err); - assert.deepStrictEqual( - response.commitTimestamp, - RESPONSE.commitTimestamp - ); - - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); - withAllSpansHaveDBName(spans); - - const actualEventNames: string[] = []; - const actualSpanNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = [ - 'CloudSpanner.Database.writeAtLeastOnce', - ]; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const firstSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.UNSET, - firstSpan.status.code, - 'Unexpected span status code' - ); - assert.strictEqual( - undefined, - firstSpan.status.message, - 'Unexpected span status message' - ); - - const expectedEventNames = ['Using Session']; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - }); - } catch (error) { - assert(error instanceof Error); - } - }); + const expectedSpanNames = ['CloudSpanner.Database.writeAtLeastOnce']; + assert.deepStrictEqual( + actualSpanNames, + expectedSpanNames, + `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` + ); + + // Ensure that the span actually produced an error that was recorded. + const firstSpan = spans[0]; + assert.strictEqual( + SpanStatusCode.UNSET, + firstSpan.status.code, + 'Unexpected span status code' + ); + assert.strictEqual( + undefined, + firstSpan.status.message, + 'Unexpected span status message' + ); + + const expectedEventNames = ['Using Session']; + assert.deepStrictEqual( + actualEventNames, + expectedEventNames, + `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` + ); + + done(); + }); + } catch (error) { + assert(error instanceof Error); + } + }); + + it('with error on null mutation should catch thrown error', done => { + try { + database.writeAtLeastOnce(null, () => {}); + } catch (err) { + // Performing a substring search on the error because + // depending on the version of Node.js, the error might be either of: + // * Cannot read properties of null (reading 'proto') + // * Cannot read property 'proto' of null + (err as grpc.ServiceError).message.includes('Cannot read propert'); + (err as grpc.ServiceError).message.includes('of null'); + + const spans = traceExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); + withAllSpansHaveDBName(spans); - it('with error on null mutation should catch thrown error', done => { - try { - database.writeAtLeastOnce(null, () => {}); - } catch (err) { - // Performing a substring search on the error because - // depending on the version of Node.js, the error might be either of: - // * Cannot read properties of null (reading 'proto') - // * Cannot read property 'proto' of null - (err as grpc.ServiceError).message.includes( - 'Cannot read propert' - ); - (err as grpc.ServiceError).message.includes('of null'); - - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); - withAllSpansHaveDBName(spans); - - const actualSpanNames: string[] = []; - const actualEventNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = [ - 'CloudSpanner.Database.writeAtLeastOnce', - ]; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const firstSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.ERROR, - firstSpan.status.code, - 'Expected an ERROR span status' - ); - - const errorMessage = firstSpan.status.message; - assert.ok( - errorMessage.includes( - "Cannot read properties of null (reading 'proto')" - ) || - errorMessage.includes("Cannot read property 'proto' of null") - ); - - // We expect an exception to have been caught as well as a Session event. - const expectedEventNames = ['Using Session', 'exception']; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - } + const actualSpanNames: string[] = []; + const actualEventNames: string[] = []; + spans.forEach(span => { + actualSpanNames.push(span.name); + span.events.forEach(event => { + actualEventNames.push(event.name); }); + }); - if (isMuxEnabled) { - it('should not retry a transaction on `Session Not Found`', done => { - const sessionNotFoundError = { - code: grpc.status.NOT_FOUND, - message: 'Session not found', - } as grpc.ServiceError; - - (sessionFactory.getSession as sinon.SinonStub).callsFake( - callback => callback(sessionNotFoundError, null, null) - ); - - database.writeAtLeastOnce(mutations, err => { - assert.deepStrictEqual(err, sessionNotFoundError); - - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); - withAllSpansHaveDBName(spans); - - const actualEventNames: string[] = []; - const actualSpanNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = [ - 'CloudSpanner.Database.writeAtLeastOnce', - ]; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const firstSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.ERROR, - firstSpan.status.code, - 'Expected an ERROR span status' - ); - assert.strictEqual( - 'Session not found', - firstSpan.status.message, - 'Mismatched span status message' - ); - - const expectedEventNames = ['No session available']; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - }); - }); - } - } - ); + const expectedSpanNames = ['CloudSpanner.Database.writeAtLeastOnce']; + assert.deepStrictEqual( + actualSpanNames, + expectedSpanNames, + `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` + ); + + // Ensure that the span actually produced an error that was recorded. + const firstSpan = spans[0]; + assert.strictEqual( + SpanStatusCode.ERROR, + firstSpan.status.code, + 'Expected an ERROR span status' + ); + + const errorMessage = firstSpan.status.message; + assert.ok( + errorMessage.includes( + "Cannot read properties of null (reading 'proto')" + ) || errorMessage.includes("Cannot read property 'proto' of null") + ); + + // We expect an exception to have been caught as well as a Session event. + const expectedEventNames = ['Using Session', 'exception']; + assert.deepStrictEqual( + actualEventNames, + expectedEventNames, + `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` + ); + + done(); + } }); }); @@ -1929,8 +1715,6 @@ describe('Database', () => { let fakeSessionFactory: FakeSessionFactory; let fakeSession: FakeSession; let fakeSession2: FakeSession; - let fakeMultiplexedSession: FakeMultiplexedSession; - let fakeMultiplexedSession2: FakeMultiplexedSession; let fakeSnapshot: FakeTransaction; let fakeSnapshot2: FakeTransaction; let fakeStream: Transform; @@ -1938,373 +1722,234 @@ describe('Database', () => { let getSessionStub: sinon.SinonStub; - const muxEnabled = [true, false]; - - muxEnabled.forEach(isMuxEnabled => { - describe( - 'when GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION is ' + - `${isMuxEnabled ? 'enabled' : 'disable'}`, - () => { - before(() => { - isMuxEnabled - ? (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION = 'true') - : (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION = - 'false'); + beforeEach(() => { + fakeSessionFactory = database.sessionFactory_; + fakeSession = new FakeSession(); + fakeSession2 = new FakeSession(); + fakeSnapshot = new FakeTransaction( + {} as google.spanner.v1.TransactionOptions.ReadOnly + ); + fakeSnapshot2 = new FakeTransaction( + {} as google.spanner.v1.TransactionOptions.ReadOnly + ); + fakeStream = through.obj(); + fakeStream2 = through.obj(); + + getSessionStub = ( + sandbox.stub(fakeSessionFactory, 'getSession') as sinon.SinonStub + ) + .onFirstCall() + .callsFake(callback => callback(null, fakeSession)) + .onSecondCall() + .callsFake(callback => callback(null, fakeSession2)); + + sandbox.stub(fakeSession, 'snapshot').returns(fakeSnapshot); + + sandbox.stub(fakeSession2, 'snapshot').returns(fakeSnapshot2); + + sandbox.stub(fakeSnapshot, 'runStream').returns(fakeStream); + + sandbox.stub(fakeSnapshot2, 'runStream').returns(fakeStream2); + + sandbox.stub(fakeSessionFactory, 'isMultiplexedEnabled').returns(false); + }); + + it('with error on `getSession`', done => { + const fakeError = new Error('getSession error'); + + getSessionStub.onFirstCall().callsFake(callback => callback(fakeError)); + + database.runStream(QUERY).on('error', err => { + assert.strictEqual(err, fakeError); + + const spans = traceExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); + withAllSpansHaveDBName(spans); + + const actualEventNames: string[] = []; + const actualSpanNames: string[] = []; + spans.forEach(span => { + actualSpanNames.push(span.name); + span.events.forEach(event => { + actualEventNames.push(event.name); }); - beforeEach(() => { - sandbox.restore(); - fakeSessionFactory = database.sessionFactory_; - fakeSnapshot = new FakeTransaction( - {} as google.spanner.v1.TransactionOptions.ReadOnly - ); - fakeSnapshot2 = new FakeTransaction( - {} as google.spanner.v1.TransactionOptions.ReadOnly - ); - fakeStream = through.obj(); - fakeStream2 = through.obj(); - - sandbox.stub(fakeSnapshot, 'runStream').returns(fakeStream); - - sandbox.stub(fakeSnapshot2, 'runStream').returns(fakeStream2); - - if (isMuxEnabled) { - fakeMultiplexedSession = new FakeMultiplexedSession(); - fakeMultiplexedSession2 = new FakeMultiplexedSession(); - getSessionStub = ( - sandbox.stub( - fakeSessionFactory, - 'getSession' - ) as sinon.SinonStub - ) - .onFirstCall() - .callsFake(callback => callback(null, fakeMultiplexedSession)) - .onSecondCall() - .callsFake(callback => callback(null, fakeMultiplexedSession2)); - - ( - sandbox.stub( - fakeMultiplexedSession, - 'snapshot' - ) as sinon.SinonStub - ).returns(fakeSnapshot); - - ( - sandbox.stub( - fakeMultiplexedSession2, - 'snapshot' - ) as sinon.SinonStub - ).returns(fakeSnapshot2); - sandbox - .stub(fakeSessionFactory, 'isMultiplexedEnabled') - .returns(true); - } else { - fakeSession = new FakeSession(); - fakeSession2 = new FakeSession(); - - getSessionStub = ( - sandbox.stub( - fakeSessionFactory, - 'getSession' - ) as sinon.SinonStub - ) - .onFirstCall() - .callsFake(callback => callback(null, fakeSession)) - .onSecondCall() - .callsFake(callback => callback(null, fakeSession2)); - - sandbox.stub(fakeSession, 'snapshot').returns(fakeSnapshot); - - sandbox.stub(fakeSession2, 'snapshot').returns(fakeSnapshot2); - - sandbox - .stub(fakeSessionFactory, 'isMultiplexedEnabled') - .returns(false); - } + }); + + const expectedSpanNames = ['CloudSpanner.Database.runStream']; + assert.deepStrictEqual( + actualSpanNames, + expectedSpanNames, + `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` + ); + + // Ensure that the span actually produced an error that was recorded. + const firstSpan = spans[0]; + assert.strictEqual( + SpanStatusCode.ERROR, + firstSpan.status.code, + 'Expected an ERROR span status' + ); + assert.strictEqual( + 'getSession error', + firstSpan.status.message, + 'Mismatched span status message' + ); + + // We don't expect events. + const expectedEventNames = []; + assert.deepStrictEqual( + actualEventNames, + expectedEventNames, + `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` + ); + + done(); + }); + }); + + it('propagation on stream/transaction errors', done => { + const fakeError = new Error('propagation err'); + const endStub = sandbox.stub(fakeSnapshot, 'end'); + + database.runStream(QUERY).on('error', err => { + assert.strictEqual(err, fakeError); + assert.strictEqual(endStub.callCount, 1); + + const spans = traceExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); + withAllSpansHaveDBName(spans); + + const actualEventNames: string[] = []; + const actualSpanNames: string[] = []; + spans.forEach(span => { + actualSpanNames.push(span.name); + span.events.forEach(event => { + actualEventNames.push(event.name); }); + }); + + const expectedSpanNames = ['CloudSpanner.Database.runStream']; + assert.deepStrictEqual( + actualSpanNames, + expectedSpanNames, + `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` + ); + + // Ensure that the span actually produced an error that was recorded. + const firstSpan = spans[0]; + assert.strictEqual( + SpanStatusCode.ERROR, + firstSpan.status.code, + 'Expected an ERROR span status' + ); + assert.strictEqual( + 'propagation err', + firstSpan.status.message, + 'Mismatched span status message' + ); + + const expectedEventNames = ['Using Session']; + assert.deepStrictEqual( + actualEventNames, + expectedEventNames, + `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` + ); + + done(); + }); + + fakeStream.destroy(fakeError); + }); + + it('retries with "Session not found" error', done => { + const sessionNotFoundError = { + code: grpc.status.NOT_FOUND, + message: 'Session not found', + } as grpc.ServiceError; + const endStub = sandbox.stub(fakeSnapshot, 'end'); + const endStub2 = sandbox.stub(fakeSnapshot2, 'end'); + let rows = 0; + + database + .runStream(QUERY) + .on('data', () => rows++) + .on('error', err => { + assert.fail(err); + }) + .on('end', async () => { + assert.strictEqual(endStub.callCount, 1); + assert.strictEqual(endStub2.callCount, 1); + assert.strictEqual(rows, 1); + + await provider.forceFlush(); + await traceExporter.forceFlush(); - it('with error on `getSession`', done => { - const fakeError = new Error('getSession error'); - - getSessionStub - .onFirstCall() - .callsFake(callback => callback(fakeError)); - - database.runStream(QUERY).on('error', err => { - assert.strictEqual(err, fakeError); - - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); - withAllSpansHaveDBName(spans); - - const actualEventNames: string[] = []; - const actualSpanNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = ['CloudSpanner.Database.runStream']; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const firstSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.ERROR, - firstSpan.status.code, - 'Expected an ERROR span status' - ); - assert.strictEqual( - 'getSession error', - firstSpan.status.message, - 'Mismatched span status message' - ); - - // We don't expect events. - const expectedEventNames = []; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); + const spans = traceExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 2, 'Exactly 2 spans expected'); + withAllSpansHaveDBName(spans); + + const actualSpanNames: string[] = []; + const actualEventNames: string[] = []; + spans.forEach(span => { + actualSpanNames.push(span.name); + span.events.forEach(event => { + actualEventNames.push(event.name); }); }); - it('propagation on stream/transaction errors', done => { - const fakeError = new Error('propagation err'); - const endStub = sandbox.stub(fakeSnapshot, 'end'); - - database.runStream(QUERY).on('error', err => { - assert.strictEqual(err, fakeError); - assert.strictEqual(endStub.callCount, 1); - - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual(spans.length, 1, 'Exactly 1 span expected'); - withAllSpansHaveDBName(spans); - - const actualEventNames: string[] = []; - const actualSpanNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = ['CloudSpanner.Database.runStream']; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const firstSpan = spans[0]; - assert.strictEqual( - SpanStatusCode.ERROR, - firstSpan.status.code, - 'Expected an ERROR span status' - ); - assert.strictEqual( - 'propagation err', - firstSpan.status.message, - 'Mismatched span status message' - ); - - const expectedEventNames = ['Using Session']; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - }); + const expectedSpanNames = [ + 'CloudSpanner.Database.runStream', + 'CloudSpanner.Database.runStream', + ]; + assert.deepStrictEqual( + actualSpanNames, + expectedSpanNames, + `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` + ); - fakeStream.destroy(fakeError); - }); + // Ensure that the span actually produced an error that was recorded. + const lastSpan = spans[0]; + assert.deepStrictEqual( + SpanStatusCode.ERROR, + lastSpan.status.code, + 'Expected an ERROR span status' + ); + assert.deepStrictEqual( + 'Session not found', + lastSpan.status.message, + 'Mismatched span status message' + ); - if (isMuxEnabled) { - it('should not retry on "Session not found" error', done => { - const sessionNotFoundError = { - code: grpc.status.NOT_FOUND, - message: 'Session not found', - } as grpc.ServiceError; - const endStub = sandbox.stub(fakeSnapshot, 'end'); - const endStub2 = sandbox.stub(fakeSnapshot2, 'end'); - let rows = 0; - - database - .runStream(QUERY) - .on('data', () => rows++) - .on('error', err => { - assert.strictEqual(err, sessionNotFoundError); - assert.strictEqual(endStub.callCount, 1); - assert.strictEqual(endStub2.callCount, 0); - assert.strictEqual(rows, 0); - - provider.forceFlush(); - traceExporter.forceFlush(); - - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual( - spans.length, - 1, - 'Exactly 1 span expected' - ); - withAllSpansHaveDBName(spans); - - const actualSpanNames: string[] = []; - const actualEventNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = ['CloudSpanner.Database.runStream']; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const lastSpan = spans[0]; - assert.deepStrictEqual( - SpanStatusCode.ERROR, - lastSpan.status.code, - 'Expected an ERROR span status' - ); - assert.deepStrictEqual( - 'Session not found', - lastSpan.status.message, - 'Mismatched span status message' - ); - - const expectedEventNames = [ - 'Using Session', - 'No session available', - ]; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - }); - - fakeStream.emit('error', sessionNotFoundError); - fakeStream2.push('row1'); - fakeStream2.push(null); - }); - } else { - it('retries with "Session not found" error', done => { - const sessionNotFoundError = { - code: grpc.status.NOT_FOUND, - message: 'Session not found', - } as grpc.ServiceError; - const endStub = sandbox.stub(fakeSnapshot, 'end'); - const endStub2 = sandbox.stub(fakeSnapshot2, 'end'); - let rows = 0; - - database - .runStream(QUERY) - .on('data', () => rows++) - .on('error', err => { - assert.fail(err); - }) - .on('end', async () => { - assert.strictEqual(endStub.callCount, 1); - assert.strictEqual(endStub2.callCount, 1); - assert.strictEqual(rows, 1); - - await provider.forceFlush(); - await traceExporter.forceFlush(); - - const spans = traceExporter.getFinishedSpans(); - assert.strictEqual( - spans.length, - 2, - 'Exactly 2 spans expected' - ); - withAllSpansHaveDBName(spans); - - const actualSpanNames: string[] = []; - const actualEventNames: string[] = []; - spans.forEach(span => { - actualSpanNames.push(span.name); - span.events.forEach(event => { - actualEventNames.push(event.name); - }); - }); - - const expectedSpanNames = [ - 'CloudSpanner.Database.runStream', - 'CloudSpanner.Database.runStream', - ]; - assert.deepStrictEqual( - actualSpanNames, - expectedSpanNames, - `span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}` - ); - - // Ensure that the span actually produced an error that was recorded. - const lastSpan = spans[0]; - assert.deepStrictEqual( - SpanStatusCode.ERROR, - lastSpan.status.code, - 'Expected an ERROR span status' - ); - assert.deepStrictEqual( - 'Session not found', - lastSpan.status.message, - 'Mismatched span status message' - ); - - // Ensure that the final span that got retries did not error. - const firstSpan = spans[1]; - assert.deepStrictEqual( - SpanStatusCode.UNSET, - firstSpan.status.code, - 'Unexpected span status code' - ); - assert.deepStrictEqual( - undefined, - firstSpan.status.message, - 'Unexpected span status message' - ); - - const expectedEventNames = [ - 'Using Session', - 'No session available', - 'Using Session', - ]; - assert.deepStrictEqual( - actualEventNames, - expectedEventNames, - `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` - ); - - done(); - }); - - fakeStream.emit('error', sessionNotFoundError); - fakeStream2.push('row1'); - fakeStream2.push(null); - }); - } - } - ); + // Ensure that the final span that got retries did not error. + const firstSpan = spans[1]; + assert.deepStrictEqual( + SpanStatusCode.UNSET, + firstSpan.status.code, + 'Unexpected span status code' + ); + assert.deepStrictEqual( + undefined, + firstSpan.status.message, + 'Unexpected span status message' + ); + + const expectedEventNames = [ + 'Using Session', + 'No session available', + 'Using Session', + ]; + assert.deepStrictEqual( + actualEventNames, + expectedEventNames, + `Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}` + ); + + done(); + }); + + fakeStream.emit('error', sessionNotFoundError); + fakeStream2.push('row1'); + fakeStream2.push(null); }); }); diff --git a/src/database.ts b/src/database.ts index 1df65246a..6f5ec17cb 100644 --- a/src/database.ts +++ b/src/database.ts @@ -2125,18 +2125,17 @@ class Database extends common.GrpcServiceObject { snapshot.begin(err => { if (err) { setSpanError(span, err); - if (isSessionNotFoundError(err)) { + if ( + isSessionNotFoundError(err) && + !this.sessionFactory_.isMultiplexedEnabled() + ) { span.addEvent('No session available', { 'session.id': session?.id, }); session!.lastError = err; this.sessionFactory_.release(session!); span.end(); - if (!this.sessionFactory_.isMultiplexedEnabled()) { - this.getSnapshot(options, callback!); - } else { - callback!(err); - } + this.getSnapshot(options, callback!); } else { span.addEvent('Using Session', {'session.id': session?.id}); this.sessionFactory_.release(session!); @@ -3102,7 +3101,8 @@ class Database extends common.GrpcServiceObject { if ( !dataReceived && - isSessionNotFoundError(err as grpc.ServiceError) + isSessionNotFoundError(err as grpc.ServiceError) && + !this.sessionFactory_.isMultiplexedEnabled() ) { // If it is a 'Session not found' error and we have not yet received // any data, we can safely retry the query on a new session. @@ -3119,13 +3119,9 @@ class Database extends common.GrpcServiceObject { dataStream.end(); snapshot.end(); span.end(); - if (!this.sessionFactory_.isMultiplexedEnabled()) { - // Create a new data stream and add it to the end user stream. - dataStream = this.runStream(query, options); - dataStream.pipe(proxyStream); - } else { - proxyStream.destroy(err); - } + // Create a new data stream and add it to the end user stream. + dataStream = this.runStream(query, options); + dataStream.pipe(proxyStream); } else { proxyStream.destroy(err); snapshot.end(); @@ -3665,18 +3661,16 @@ class Database extends common.GrpcServiceObject { return startTrace('Database.writeAtLeastOnce', this._traceConfig, span => { this.sessionFactory_.getSession((err, session?, transaction?) => { - if (err && isSessionNotFoundError(err as grpc.ServiceError)) { + if ( + err && + isSessionNotFoundError(err as grpc.ServiceError) && + !this.sessionFactory_.isMultiplexedEnabled() + ) { span.addEvent('No session available', { 'session.id': session?.id, }); - if (!this.sessionFactory_.isMultiplexedEnabled()) { - this.writeAtLeastOnce(mutations, options, cb!); - } else { - setSpanError(span, err); - span.end(); - cb!(err as grpc.ServiceError); - } span.end(); + this.writeAtLeastOnce(mutations, options, cb!); return; } if (err) { diff --git a/src/session-factory.ts b/src/session-factory.ts index 7fefc7d70..1559176bc 100644 --- a/src/session-factory.ts +++ b/src/session-factory.ts @@ -113,7 +113,7 @@ export class SessionFactory this.pool_.on('error', this.emit.bind(database, 'error')); this.pool_.open(); this.multiplexedSession_ = new MultiplexedSession(database); - // set the isMulttiplexed property to true if multiplexed session is enabled, otherwise set the property to false + // set the isMultiplexed property to true if multiplexed session is enabled, otherwise set the property to false process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS === 'true' ? (this.isMultiplexed = true) : (this.isMultiplexed = false); diff --git a/test/database.ts b/test/database.ts index 20765a11f..3dc1959ce 100644 --- a/test/database.ts +++ b/test/database.ts @@ -129,18 +129,6 @@ export class FakeMultiplexedSession extends EventEmitter { } createSession() {} getSession() {} - snapshot(): FakeTransaction { - return new FakeTransaction( - {} as google.spanner.v1.TransactionOptions.ReadOnly - ); - } - isMultiplexedEnabled(): boolean { - if (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS === 'false') { - return false; - } else { - return true; - } - } } export class FakeSessionFactory extends EventEmitter { @@ -149,13 +137,7 @@ export class FakeSessionFactory extends EventEmitter { super(); this.calledWith_ = arguments; } - getSession(): FakeSession | FakeMultiplexedSession { - if (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS === 'false') { - return new FakeSession(); - } else { - return new FakeMultiplexedSession(); - } - } + getSession() {} getPool(): FakeSessionPool { return new FakeSessionPool(); } @@ -292,9 +274,9 @@ describe('Database', () => { './codec': {codec: fakeCodec}, './partial-result-stream': {partialResultStream: fakePartialResultStream}, './session-pool': {SessionPool: FakeSessionPool}, + './multiplexed-session': {MultiplexedSession: FakeMultiplexedSession}, './session-factory': {SessionFactory: FakeSessionFactory}, './session': {Session: FakeSession}, - './multiplexed-session': {MultiplexedSession: FakeMultiplexedSession}, './table': {Table: FakeTable}, './transaction-runner': { TransactionRunner: FakeTransactionRunner, @@ -836,10 +818,7 @@ describe('Database', () => { Thing: 'xyz', }); - const muxEnabled = [true, false]; - const SESSION = new FakeSession(); - const MULTIPLEXEDSESSION = new FakeMultiplexedSession(); const RESPONSE = {commitTimestamp: {seconds: 1, nanos: 0}}; const TRANSACTION = new FakeTransaction( {} as google.spanner.v1.TransactionOptions.ReadWrite @@ -847,6 +826,8 @@ describe('Database', () => { let sessionFactory: FakeSessionFactory; + const muxEnabled = [true, false]; + muxEnabled.forEach(isMuxEnabled => { describe( 'when GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION is ' + @@ -858,23 +839,17 @@ describe('Database', () => { : (process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION = 'false'); }); + beforeEach(() => { sandbox.restore(); sessionFactory = database.sessionFactory_; - if (isMuxEnabled) { - ( - sandbox.stub(sessionFactory, 'getSession') as sinon.SinonStub - ).callsFake(callback => { - callback(null, MULTIPLEXEDSESSION, TRANSACTION); - }); - } else { - ( - sandbox.stub(sessionFactory, 'getSession') as sinon.SinonStub - ).callsFake(callback => { - callback(null, SESSION, TRANSACTION); - }); - } + ( + sandbox.stub(sessionFactory, 'getSession') as sinon.SinonStub + ).callsFake(callback => { + callback(null, SESSION, TRANSACTION); + }); }); + it('should return any errors getting a session', done => { const fakeErr = new Error('err'); @@ -1937,8 +1912,6 @@ describe('Database', () => { let fakeSessionFactory: FakeSessionFactory; let fakeSession: FakeSession; let fakeSession2: FakeSession; - let fakeMultiplexedSession: FakeMultiplexedSession; - let fakeMultiplexedSession2: FakeMultiplexedSession; let fakeSnapshot: FakeTransaction; let fakeSnapshot2: FakeTransaction; let fakeStream: Transform; @@ -1962,8 +1935,9 @@ describe('Database', () => { 'false'); }); beforeEach(() => { - sandbox.restore(); fakeSessionFactory = database.sessionFactory_; + fakeSession = new FakeSession(); + fakeSession2 = new FakeSession(); fakeSnapshot = new FakeTransaction( {} as google.spanner.v1.TransactionOptions.ReadOnly ); @@ -1972,68 +1946,30 @@ describe('Database', () => { ); fakeStream = through.obj(); fakeStream2 = through.obj(); - if (isMuxEnabled) { - fakeMultiplexedSession = new FakeMultiplexedSession(); - fakeMultiplexedSession2 = new FakeMultiplexedSession(); - - getSessionStub = ( - sandbox.stub( - fakeSessionFactory, - 'getSession' - ) as sinon.SinonStub - ) - .onFirstCall() - .callsFake(callback => callback(null, fakeMultiplexedSession)) - .onSecondCall() - .callsFake(callback => callback(null, fakeMultiplexedSession2)); - - snapshotStub = sandbox - .stub(fakeMultiplexedSession, 'snapshot') - .returns(fakeSnapshot); - - sandbox - .stub(fakeMultiplexedSession2, 'snapshot') - .returns(fakeSnapshot2); - - runStreamStub = sandbox - .stub(fakeSnapshot, 'runStream') - .returns(fakeStream); - - sandbox.stub(fakeSnapshot2, 'runStream').returns(fakeStream2); - - sandbox - .stub(fakeSessionFactory, 'isMultiplexedEnabled') - .returns(true); - } else { - fakeSession = new FakeSession(); - fakeSession2 = new FakeSession(); - getSessionStub = ( - sandbox.stub( - fakeSessionFactory, - 'getSession' - ) as sinon.SinonStub - ) - .onFirstCall() - .callsFake(callback => callback(null, fakeSession)) - .onSecondCall() - .callsFake(callback => callback(null, fakeSession2)); - snapshotStub = sandbox - .stub(fakeSession, 'snapshot') - .returns(fakeSnapshot); + getSessionStub = ( + sandbox.stub(fakeSessionFactory, 'getSession') as sinon.SinonStub + ) + .onFirstCall() + .callsFake(callback => callback(null, fakeSession)) + .onSecondCall() + .callsFake(callback => callback(null, fakeSession2)); - sandbox.stub(fakeSession2, 'snapshot').returns(fakeSnapshot2); + snapshotStub = sandbox + .stub(fakeSession, 'snapshot') + .returns(fakeSnapshot); - runStreamStub = sandbox - .stub(fakeSnapshot, 'runStream') - .returns(fakeStream); + sandbox.stub(fakeSession2, 'snapshot').returns(fakeSnapshot2); - sandbox.stub(fakeSnapshot2, 'runStream').returns(fakeStream2); + runStreamStub = sandbox + .stub(fakeSnapshot, 'runStream') + .returns(fakeStream); - sandbox - .stub(fakeSessionFactory, 'isMultiplexedEnabled') - .returns(false); - } + sandbox.stub(fakeSnapshot2, 'runStream').returns(fakeStream2); + + sandbox + .stub(fakeSessionFactory, 'isMultiplexedEnabled') + .returns(isMuxEnabled ? true : false); }); it('should get a read session via `getSession`', () => { @@ -2115,7 +2051,9 @@ describe('Database', () => { database.runStream(QUERY).on('error', err => { assert.strictEqual(err, sessionNotFoundError); assert.strictEqual(endStub.callCount, 1); + // make sure it is not retrying the stream assert.strictEqual(endStub2.callCount, 0); + // row count should be 0 assert.strictEqual(rows, 0); done(); }); @@ -2417,7 +2355,6 @@ describe('Database', () => { describe('getSnapshot', () => { let fakeSessionFactory: FakeSessionFactory; let fakeSession: FakeSession; - let fakeMultiplexedSession: FakeMultiplexedSession; let fakeSnapshot: FakeTransaction; let beginSnapshotStub: sinon.SinonStub; @@ -2440,6 +2377,7 @@ describe('Database', () => { beforeEach(() => { fakeSessionFactory = database.sessionFactory_; + fakeSession = new FakeSession(); fakeSnapshot = new FakeTransaction( {} as google.spanner.v1.TransactionOptions.ReadOnly ); @@ -2448,62 +2386,22 @@ describe('Database', () => { sandbox.stub(fakeSnapshot, 'begin') as sinon.SinonStub ).callsFake(callback => callback(null)); - if (isMuxEnabled) { - fakeMultiplexedSession = new FakeMultiplexedSession(); - getSessionStub = ( - sandbox.stub( - fakeSessionFactory, - 'getSession' - ) as sinon.SinonStub - ).callsFake(callback => callback(null, fakeMultiplexedSession)); - - snapshotStub = ( - sandbox.stub( - fakeMultiplexedSession, - 'snapshot' - ) as sinon.SinonStub - ).returns(fakeSnapshot); + getSessionStub = ( + sandbox.stub(fakeSessionFactory, 'getSession') as sinon.SinonStub + ).callsFake(callback => callback(null, fakeSession)); - ( - sandbox.stub( - fakeSessionFactory, - 'isMultiplexedEnabled' - ) as sinon.SinonStub - ).returns(true); - } else { - fakeSession = new FakeSession(); - getSessionStub = ( - sandbox.stub( - fakeSessionFactory, - 'getSession' - ) as sinon.SinonStub - ).callsFake(callback => callback(null, fakeSession)); - - snapshotStub = sandbox - .stub(fakeSession, 'snapshot') - .returns(fakeSnapshot); + snapshotStub = ( + sandbox.stub(fakeSession, 'snapshot') as sinon.SinonStub + ).returns(fakeSnapshot); - ( - sandbox.stub( - fakeSessionFactory, - 'isMultiplexedEnabled' - ) as sinon.SinonStub - ).returns(false); - } + ( + sandbox.stub( + fakeSessionFactory, + 'isMultiplexedEnabled' + ) as sinon.SinonStub + ).returns(isMuxEnabled ? true : false); }); - it( - 'should call through to ' + - `${isMuxEnabled ? 'MultiplexedSession#getSession' : 'SessionPool#getSession'}`, - () => { - getSessionStub.callsFake(() => {}); - - database.getSnapshot(assert.ifError); - - assert.strictEqual(getSessionStub.callCount, 1); - } - ); - it( 'should return any ' + `${isMuxEnabled ? 'multiplexed session' : 'pool'}` + @@ -2562,14 +2460,6 @@ describe('Database', () => { assert.strictEqual(bounds, fakeTimestampBounds); }); - it('should begin a snapshot', () => { - beginSnapshotStub.callsFake(() => {}); - - database.getSnapshot(assert.ifError); - - assert.strictEqual(beginSnapshotStub.callCount, 1); - }); - it('should return the `snapshot`', done => { database.getSnapshot((err, snapshot) => { assert.ifError(err); @@ -2630,6 +2520,7 @@ describe('Database', () => { .callsFake(callback => callback(null, fakeSession)) .onSecondCall() .callsFake(callback => callback(null, fakeSession2)); + beginSnapshotStub.callsFake(callback => callback(fakeError)); // The first session that was not found should be released back into the @@ -2677,7 +2568,6 @@ describe('Database', () => { let getSessionStub: sinon.SinonStub; beforeEach(() => { - process.env.GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSION = 'false'; fakePool = database.pool_; fakeSessionFactory = database.sessionFactory_; fakeSession = new FakeSession(); diff --git a/test/spanner.ts b/test/spanner.ts index 769257e27..ec2398c9b 100644 --- a/test/spanner.ts +++ b/test/spanner.ts @@ -1744,7 +1744,7 @@ describe('Spanner with mock server', () => { } }); - it('should fail the transaction, if query return session not found error', done => { + it('should fail the transaction, if query returns session not found error', done => { const query = { sql: selectSql, } as ExecuteSqlRequest;